Closed Bug 886046 Opened 8 years ago Closed 8 years ago

Add a MockColorPicker module in SpecialPowers

Categories

(Testing :: General, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla25

People

(Reporter: mounir, Assigned: mounir)

References

Details

Attachments

(1 file, 1 obsolete file)

Attached patch Patch (obsolete) — Splinter Review
No description provided.
Attachment #766337 - Flags: review?(ctalbert)
Comment on attachment 766337 [details] [diff] [review]
Patch

Review of attachment 766337 [details] [diff] [review]:
-----------------------------------------------------------------

This looks pretty good. I think my only nit is that the exception you throw in open should start with TEST-UNEXPECTED-FAIL | <issue> so that it shows up in the test logs (it will be highlighted as a failure).

Ideally, I'd love to see a test for this (to ensure the mock object properly restores the real colorpicker on cleanup, but I don't see any way to easily verify that, so I'll leave that one up to you.  The other mock objects also don't have these types of tests. (Tests for specialpowers should be in testing/mochitest/tests if you think of a good way to add one)
Attachment #766337 - Flags: review?(ctalbert) → review+
Attached patch PatchSplinter Review
I did a few changes. Some being related to nsIColorPicker interface changes and some more related to the test infra: if there is no component, the test will no longer fail and it will just use the mock instead. It is very useful to be able to land stuff without all backends being implemented.
Attachment #766337 - Attachment is obsolete: true
Attachment #772089 - Flags: review?(ctalbert)
Comment on attachment 772089 [details] [diff] [review]
Patch

Review of attachment 772089 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good. I like it better. The only issue I see (and I'm not sure it is an issue) is that in the previous patch you had a returnOK and returnCancel constants on the MockColorPicker. Not sure if the spec changed and you left them off intentionally or if it was an omission.

Otherwise, it looks good. I like this better.
Attachment #772089 - Flags: review?(ctalbert) → review+
(In reply to Clint Talbert ( :ctalbert ) from comment #3)
> Comment on attachment 772089 [details] [diff] [review]
> Patch
> 
> Review of attachment 772089 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks good. I like it better. The only issue I see (and I'm not sure it
> is an issue) is that in the previous patch you had a returnOK and
> returnCancel constants on the MockColorPicker. Not sure if the spec changed
> and you left them off intentionally or if it was an omission.

We change the interface so those things are no longer there. Thanks for the quick review :)
https://hg.mozilla.org/mozilla-central/rev/cc0b2e977847
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Component: Infrastructure → General
You need to log in before you can comment on or make changes to this bug.