Issue #1664: Unit Test support for mapserver

[Umberto Nicoletti]

Sorry for the late reply, but I have been busy and I needed time to
meditate on the answers...

On 2/17/06, Frank Warmerdam <warmerdam at pobox.com> wrote:
> Umberto Nicoletti wrote:
> > I have published a first public draft of rfc-12: Unit tests
> >
> > http://mapserver.gis.umn.edu/development/rfc/ms-rfc-12
> >
> > Please share your thoughts.
>
> Umberto,
>
> Thanks for the write up.
>
> My first concern is that this statement:
>
> "Adopting unit tests can be summarized by the following two rules:
>
>     1. first write the tests, then write the code
>     2. cannot commit unless all tests pass"
>
> implies a dramatic change development style on MapServer and presupposes
> a level of control over how developers work that has never been contemplated
> before.  Heck, I don't think we were even able to agree on a coding style.
> In fact what you are doing is proposing that MapServer switch wholesale to
> Extreme Programming.  I personally feel this is a leap too far.
>

Unit testing is not extreme programming, because xp implies a lot more things:
pair programming, customer on site, story cards, etc. On the other hand
XP implies unit testing.

Unit testing (and Test Driven Design) are in fact much more popular
and sucessfull
than xp as the ratio between required mind shift, impact on the
usual programming practices and effort put into the transition is
generally lower.

The idea that I am trying to pass on is that when we write code we usually do
the testing anyway, but most of the times this requires a human (that is us)
to run the program and check the results manually, so it is an improvement
if we could automate this task, for example by writing a program to do it.

On the short period this is probably overkilling, but on a medium to long term
it will pay off largely.


If any of you has ever written a Perl module then you know that almost all
Perl modules have a more or less extensive suite of tests that is run
at build time
And this is so since a long time (probably before xp or unit testing):
it seems to me that this hasn't kept skilled coders or spare-time-coders away
from Perl (Perl does require skills anyway, even only to understand
code you wrote
two weeks ago :-) ).

I agree that extreme programming is a leap too far.

> I think a more moderate, but still fairly ambitious approach might be to
> require folks preparing RFCs to describe new function entry points they
> are proposing, and describing how these would be unit tested.  Then
> requiring that the unit tests for those entry points be implemented and
> passing before the corresponding code is committed.
>
> Whether the developer writes the unit test before or after the actual code
> would be up to them.

As long as they write tests.

>
> "Always write tests: to document an existing bug and its resolution,
> before coding new features, etc."
>
> Just who do you think is going to write all these unit tests?  Users
> are not normally going to prepare a unit test to document a bug.  I am
> damn lucky if I can get them to submit a map+data I can actually run to
> reproduce the problem.  Are you expecting developers to write these
> test cases before they take on fixing a bug?  I have a news flash.  A lot
> of the bug fixing done in MapServer is done out of the goodness of our
> hearts on unfunded time and we are already doing a pretty poor job of
> staying on top of the bug reports.  If you raise the bar for the work
> involved in addressing a bug, you are likely to just lose a bunch of
> bug fixers (ie. me).
>

Unit testing is not more work: is simply doing what you are probably doing
anyway (you do test your modifications already before committing, don't you?),
only in an automated fashion.
Of course it will not always be easy, but we will work out a solution together.

Lots of perl coders do that already, so it seems feasible and desirable to me.

> There are definitely times though when a bug fix that is committed ought
> to be accompanied by a unit tests, and this should be encouraged without
> a firm requirement.  It is routine for me to prepare an msautotest in
> response to a bug fix that I think is a big scary or fragile.
>
> "Developers must use the usual preprocessor flags to enable conditional
> compiling of unit tests. This is necessary to avoid test failures due to a
> missing feature (as is the case, for instance of the Python unit tests when
> compiled without gdal)."
>
> I assume here your point is that the unit tests need to be surrounded
> with #ifdef USE_PROJ and similar conditionals to ensure unit tests for
> features that aren't compiled in executed.  Is that right?  This seems
> reasonable.  It would be nice if reports skipped in this way could be
> reported as skipped in order to give some sense of coverage.
>

I see we agree on this one.

> "The configure script must be updated to reflect this new dependency
> and a big red warning sign should be displayed when unit testing is
> not available (even though this could confuse even more users)."
>
> I'm not sure what you mean by the above.  I guess that if cunit is
> not found, that the configure script should report the issue?  That
> is reasonable, though rather than making it seem like something really
> scary is wrong I think configure can just report something like
> "cunit not found, C unit testing disabled."

Exactly that, but I would put emphasys on this missing feature.
Something like this:

snippet of configure output:

[configure ends....]

You have successfully configured Mapserver version Z.Y.X
The following features will be enabled:
              * thread safe support (required for Java mapscript)
              * gdal
              * ogr
              * proj
              * etc...

CUnit was not found on your system, so unit tests have been disabled.
If you are building mapserver from cvs the mapserver devs strongly suggest
you run the unit tests to make sure you have a functional mapserer.

/snippet

>
> Some features can only be effectively tested by analyzing the
> produced image.  Do you have any thoughts on how to accomplish this
> in a unit testing scenario?
>

We could copy from the Perl GD module, just to get an idea.

> By far my biggest concern is that C unit testing, especially at
> the level of granularity you seem to be suggesting, would seem to
> make some kinds of changes very difficult.  If we have thousands
> of unit tests setup, and then we want to restructure the mapObj somewhat
> are we going to be stuck in a position where doing so requires reviewing
> and updating many of the unit tests?
>
> Recently Assefa made a small improvement in how a default situations
> in WMS was handled with regard to aspect ratio.  It turned out this
> caused almost all the msautotest/wxs tests to fail and I spent a whole
> morning carefully reviewing the new output results of them all, and
> accepting them.  If we have too much "weight" of test framework setup
> we run the risk of making such improvements to costly to apply.  Or
> worse yet, of our test suites falling into disuse because no one is
> willing to update them.

<joke>
In this case even the msautotest suite
looks like more "weight" and its adoption should not be pursued anymore ;-).
</joke>

Serious again:
It is exactly in this cases that tests (either unit of functional
tests) prove their value: now suppose that I am about to refactor mapObj.

Assume that I expect my changes not to impact posgtres,
sde, mysql, shape, etc code. In this case the tests for them must
continue to pass.

After I'm done with the refactoring I just run the tests and see postgres tests
fail (besides some, expected, about mapObj). What do I do now is: I fix it
or ask someone else to help me with it.
But if I didn't know it failed because I did not have unit tests
to run I'll be likely to commit changes with bugs in it.

I see your point anyway, that's why I was strict on the need of
writing tests and
making sure they all passed!

>
> Finally, I would really appreciate a more concrete sense of how this
> would all work.  Could you possible setup a patch we could apply to
> our local trees demonstrating how this in action?

There is a patch attached to issue #1664 in bugzilla.

Note that it only took 6 hours to me to write that patch (and I am not
usually programming
in C nowadays ;-) ).

Best regards,
Umberto

>
> Note, as long as the RFC is essentially trying to force the whole
> team to use XP practices I will be a -1.  As long as it has statements
> like "Always write tests: to document an existing bug and its resolution,
> before coding new features, etc."  I will also likely be a -1.
>
> In my opinion attempting to completely overhaul the way stuff gets done
> has a very high bar of approval.  So, I would suggest somewhat more
> modest goals.  If we get comfortable with the process while it still
> includes a reasonable level of developer discretion, then we might be
> willing to strengthen the requirements.
>
> Best regards,
> --
> ---------------------------------------+--------------------------------------
> I set the clouds in motion - turn up   | Frank Warmerdam, warmerdam at pobox.com
> light and sound - activate the windows | http://pobox.com/~warmerdam
> and watch the world go round - Rush    | Geospatial Programmer for Rent
>
>