Unit test for great justice
Created attachment 43938 [details] Patch
Comment on attachment 43938 [details] Patch I don't understand why you're adding "return tool": 43 return tool Why? 232 bugs_to_patches[bug_id] = bugs_to_patches.get(bug_id, []).append(patch) 234 bugs_to_patches[bug_id] = bugs_to_patches.get(bug_id, []) + [patch] Does .append() return none? Was it not working before? I'm not sure I understand: 36 def _default_options(self): either, but it's nice to list them all. Why does MockBugzilla inherit from Mock? 34 class MockBugzilla(Mock): I'm not saying it shouldn't, just not sure what that adds. I would think that we would make: 35 patch1 = { into an array eventually. THen the fetch_attachment lookup is super-simple. Is this gonna be a problem? 93 self.checkout_root = os.getcwd() Does it ever touch the disk? Will it end up leaving turd files as a result of the test, or fail if you run-webkit-tests from a non-scm directory? Otherwise looks OK. The return and .append change are at least un-explained. The return tool change might be "wrong".
> I don't understand why you're adding "return tool": > 43 return tool > Why? Yeah, this isn't needed until the future. I'll add it then. > 232 bugs_to_patches[bug_id] = bugs_to_patches.get(bug_id, > []).append(patch) > 234 bugs_to_patches[bug_id] = bugs_to_patches.get(bug_id, []) + > [patch] > > Does .append() return none? Yes. > Was it not working before? Right, it was broken if you had multiple patches per bug. > I'm not sure I understand: > 36 def _default_options(self): > either, but it's nice to list them all. Basically, I want to test the default path through each command, so i have to use an options object with the default values. > Why does MockBugzilla inherit from Mock? > 34 class MockBugzilla(Mock): So I don't have have implement all the functions that return None. Mock takes care of that more me. Eventually we should assert which functions actually got used, but that's for the future. > I would think that we would make: > 35 patch1 = { > into an array eventually. THen the fetch_attachment lookup is super-simple. Ok. > Is this gonna be a problem? > 93 self.checkout_root = os.getcwd() > Does it ever touch the disk? Will it end up leaving turd files as a result of > the test, or fail if you run-webkit-tests from a non-scm directory? As far as I know, this should be fine. The issue is that some of the commands cd to this directory, so we need something real.
Created attachment 43957 [details] download_unittest
Comment on attachment 43957 [details] download_unittest 76 raise Exception("Bogus attachment_id in fetch_attachment.") Doesn't actually match what the original would do (which in some cases we actually would want to test). But we can cover that when we actually test bugzilla.py against some sort of bugzilla-like instance.
Comment on attachment 43957 [details] download_unittest Clearing flags on attachment: 43957 Committed r51442: <http://trac.webkit.org/changeset/51442>
All reviewed patches have been landed. Closing bug.