Code review comment for lp:~dooferlad/linaro-ci-dashboard/add_api_tastypie

Revision history for this message
Stevan Radaković (stevanr) wrote :

Hey James,

Nice work rly.. I've managed to run everything pretty much smooth, tests worked, and the code looks good as well, disregarding the problem with django version for unit testing, this is really unfortunate dependency :/

First thing that kinda bothers me is that we require 'v1/' in API links. I read the tastypie docs a bit but couldn't get quick answer if this is easy to remove or not.. If there's any special reason to keep it tho, I'd like to hear it.

Second thing is just a minor code block in tests that stung me in the eye, but nothing special:

280 + # One loop is of type AndroidLoop, the other ModelBase. Find which is
281 + # which.
282 + if json[0]['type'] == 'AndroidLoop':
283 + android_loop_index = 0
284 + base_loop_index = 1
285 + else:
286 + android_loop_index = 1
287 + base_loop_index = 0
288 +
289 + # Now test the contents of the loop data
290 + self.assertEqual(json[android_loop_index]['name'],
291 + self.android_loop.name)
292 + self.assertEqual(json[base_loop_index]['name'],
293 + self.loop.name)

Why not use sets here, like

api_test_names = {json[0]['name'], json[1]['name']}
test_names = {self.android_loop.name, self.loop.name}
self.assertEqual(api_test_names, test_names)

« Back to merge proposal