launchpad-reviewers team mailing list archive

["Jeroen T. Vermeulen" <jtv@xxxxxxxxxxxxx>]

The proposal to merge lp:~jtv/gomaasapi/test-upload-file into lp:gomaasapi has been updated.

Description changed to:

This was a silly one, but it took me a few days to figure out because I was hitting the problem in another project, when really the gomaasapi test suite should have caught it first.

The handler in the test service that receives a file upload was not receiving the required "filename" parameter.  It always used the empty string instead.  We did not notice this because originally the handler did not check for the case that the parameter might be missing (and obviously the test suite did not cover this case), and file uploads were not covered by the test suite.

Once I had a test, it was easy to see what went wrong: instead of looking in the http request's Form field (which holds both POST-style and GET-style parameters), the code re-parsed the query part of the raw, unparsed version of the request's URL and looked for a GET-style parameter there.  The requesting code actually submits it as a POST parameter.  It might have been nice for debugging to pass this particular parameter as part of the URL, but we don't currently have a way to tell the client which parameters to pass as GET and which to pass as POST (and we probably don't want all parameters as GET).  There is a test for parameter-passing, but the parameter-passing code that ran here was hidden from the tests by what I call the "Swiss Army Knife" antipattern.

There are a few lessons we can learn from this:

 * Go does not have exceptions.  Check your error conditions, or debugging turns into a nightmare.

 * Code changes.  You may know that something doesn't fail now, but you still need to check in case somebody breaks it in the future.

 * Distrust third-party input.  Check for obvious mistakes.

 * Avoid unnecessary cyclomatic complexity.  It hides problems from both tests and humans.

 * Particularly avoid "if" clauses that are there just to reconstruct the intent of the calling code, if the intent was already known and constant at the call site.  This is the Swiss Army Knife function: it's both a weak knife and a wobbly screwdriver.  Better to accept a bit more code at the call site, and look for other ways to avoid repetition.

 * Unit-testing and integration-testing are both necessary.  Integration-testing should cover a basic happy path and error handling, unit-testing can go through the nooks and crannies.  But that doesn't work well with the Swiss Army Knife, because it needs to integrate multiple code paths at the call level with multiple code paths at the callee level.

 * If it's not tested, it's broken.  Wherever possible, test before implementing so that you can see the test go from failure to success.

 * Fixing bugs as you come across them is disruptive and makes for unpredictable costs.  Slower development earlier on could have saved us time in the current stage.


Jeroen

For more details, see:
https://code.launchpad.net/~jtv/gomaasapi/test-upload-file/+merge/151433
-- 
https://code.launchpad.net/~jtv/gomaasapi/test-upload-file/+merge/151433
Your team MAAS Maintainers is requested to review the proposed merge of lp:~jtv/gomaasapi/test-upload-file into lp:gomaasapi.