Bug 31923 – [bzt] Unit test download commands

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Adam Barth]

Unit test for great justice

[Adam Barth]

Created attachment 43938 [details]
Patch

[Eric Seidel (no email)]

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

[Adam Barth]

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

[Adam Barth]

Created attachment 43957 [details]
download_unittest

[Eric Seidel (no email)]

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.

[Adam Barth]

Comment on attachment 43957 [details]
download_unittest

Clearing flags on attachment: 43957

Committed r51442: <http://trac.webkit.org/changeset/51442>

[Adam Barth]

All reviewed patches have been landed.  Closing bug.