Jeroen T. Vermeulen has proposed merging lp:~jtv/gomaasapi/test-upload-file
into lp:gomaasapi.
Commit message:
Test the test service's handling of file uploads, and fix handling of the
"filename" parameter.
Requested reviews:
MAAS Maintainers (maas-maintainers)
For more details, see:
https://code.launchpad.net/~jtv/gomaasapi/test-upload-file/+merge/151433
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.
Jeroen
--
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.
=== modified file 'testservice.go'
--- testservice.go 2013-03-04 02:50:16 +0000
+++ testservice.go 2013-03-04 09:02:22 +0000
@@ -420,9 +420,7 @@
err := r.ParseMultipartForm(10000000)
checkError(err)
- values, err := url.ParseQuery(r.URL.RawQuery)
- checkError(err)
- filename := values.Get("filename")
+ filename := r.Form.Get("filename")
if filename == "" {
panic("upload has no filename")
}
=== modified file 'testservice_test.go'
--- testservice_test.go 2013-03-04 02:50:16 +0000
+++ testservice_test.go 2013-03-04 09:02:22 +0000
@@ -420,3 +420,22 @@
operations := nodeOperations["mysystemid"]
c.Check(operations, DeepEquals, []string{"start"})
}
+
+func (suite *TestMAASObjectSuite) TestUploadFile(c *C) {
+ const filename = "myfile.txt"
+ const fileContent = "uploaded contents"
+ files := suite.TestMAASObject.GetSubObject("files")
+ params := url.Values{"filename": {filename}}
+ filesMap := map[string][]byte{"file": []byte(fileContent)}
+
+ // Upload a file.
+ _, err := files.CallPostFiles("add", params, filesMap)
+ c.Assert(err, IsNil)
+
+ // The file can now be downloaded.
+ downloadedFile, err := files.CallGet("get", params)
+ c.Assert(err, IsNil)
+ bytes, err := downloadedFile.GetBytes()
+ c.Assert(err, IsNil)
+ c.Check(string(bytes), Equals, fileContent)
+}
_______________________________________________
Mailing list: https://launchpad.net/~launchpad-reviewers
Post to : [email protected]
Unsubscribe : https://launchpad.net/~launchpad-reviewers
More help : https://help.launchpad.net/ListHelp
