Issue #1664: Unit Test support for mapserver

[Frank Warmerdam]

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.

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.

"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).

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.

"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."

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?

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.

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?

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