Closed Bug 1428844 Opened 3 years ago Closed 3 years ago

Use BrowserTestUtils.waitForEvent in preferences tests

Categories

(Firefox :: Preferences, enhancement, P5)

Product:
Firefox
Component:
Preferences
Version:
58 Branch
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 60
Tracking Flags:
Tracking Status
firefox60 --- fixed

People

(Reporter: johannh, Assigned: arjunkrishnababu96, Mentored)

Details

Attachments

(2 files)

bug1428844-diff.patch
4.87 KB, patch
johannh
: feedback+
Details | Diff | Splinter Review
Bug 1428844 - Use BrowserTestUtils.waitForEvent in preferences tests;
59 bytes, text/x-review-board-request
johannh
: review+
Details 
There's a waitForEvent function in preferences/in-content/tests/head.js[0] which is not needed because we can simply use BrowserTestUtils.waitForEvent[1] instead.

We need to delete the function and replace all its occurrences[2] with BrowserTestUtils.waitForEvent.

[0] https://searchfox.org/mozilla-central/rev/f42618c99dcb522fb674221acfbc68c2d92e7936/browser/components/preferences/in-content/tests/head.js#97
[1] https://searchfox.org/mozilla-central/rev/f42618c99dcb522fb674221acfbc68c2d92e7936/testing/mochitest/BrowserTestUtils/BrowserTestUtils.jsm#788
[2] https://searchfox.org/mozilla-central/search?q=%5Cswaitforevent&case=false&regexp=true&path=preferences
I'd like to work on this. Could you please point me in the right direction?
Thanks!
Flags: needinfo?(jhofmann)
Hi Arjun,

thanks for your help! I've assigned you to the bug. Most of what you need to know is already in comment 0. This is mostly about replacing "waitForEvent" with "BrowserTestUtils.waitForEvent" in the specified test files:

https://searchfox.org/mozilla-central/search?q=%5Cswaitforevent&case=false&regexp=true&path=preferences

You can find some general instructions on how to get started with building Firefox here:

https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Introduction

Let me know if you have any other questions!
Assignee: nobody → arjunkrishnababu96
Status: NEW → ASSIGNED
Flags: needinfo?(jhofmann)
As per comment 0, I replaced all occurences of waitForEvent with BrowserTestUtils.waitForEvent.

In comment 0, you also mentioned:
> We need to delete the function and replace all its occurrences[2] with BrowserTestUtils.waitForEvent.
Should I actually *remove* the definition of waitForEvent in preferences/in-content/tests/head.js ? 
Or should I keep the function definition lying around?

Anyway, I've attached the hg diff output as a .patch file so that you can see what I've done so far. Let me know if it looks good, or if there are further changes I need to make, and I'll make them and possibly put in a mozreview request.
Flags: needinfo?(jhofmann)
Comment on attachment 8943089 [details] [diff] [review]
bug1428844-diff.patch

Hey, that looks great so far. Yes, please remove the function definition (since we're not using that function anymore, if I'm not mistaken).

Did you run the tests already? You can run a test by using ./mach mochitest, e.g.

./mach mochitest browser/components/preferences/in-content/tests/browser_connection.js

Feel free to flag me for review either in Bugzilla or through mozreview when that's done :)

Thanks!
Flags: needinfo?(jhofmann)
Attachment #8943089 - Flags: feedback+
Comment on attachment 8943677 [details]
Bug 1428844 - Use BrowserTestUtils.waitForEvent in preferences tests;

https://reviewboard.mozilla.org/r/214066/#review219974

Looks good to me, thanks! Let's give this a run on our Try server to check if the tests are still passing on automation. I'll also set the checkin-needed flag to get the patch checked in when Try is happy.
Attachment #8943677 - Flags: review?(jhofmann) → review+
I'm a newbie and I'm asking out of curiosity: what happens now?
Hey, turns out that there are Try failures (https://treeherder.mozilla.org/#/jobs?repo=try&revision=4a9fc4d2e8e86338995fd83cea7cdcd84ad23388), and I think they're actually caused by something I ran into while working on bug 1422163. I think the code in browser_siteData.js doesn't get the "test-indexedDB-done" event correctly because of the way it is dispatched here:

https://searchfox.org/mozilla-central/rev/e9a067a401e41017766f4ab90bc3906603273d51/browser/components/preferences/in-content/tests/site_data_test.html#24

Please run

./mach mochitest browser/components/preferences/in-content/tests/browser_siteData.js

to confirm that the test timeouts for you locally as well.

My fix for this in bug 1422163 was to change site_data_test.html to use this line instead:

tx.oncomplete = () => document.dispatchEvent(new CustomEvent("test-indexedDB-done", {bubbles: true, cancelable: false}));

So you could either do that or, more simply, just don't replace the waitForEvent function in browser_siteData.js and don't delete it from head.js and leave the removal to me in bug 1422163. I'd accept a patch that does either :)

Sorry for the hassle, let me know what you think!
(In reply to Johann Hofmann [:johannh] from comment #8)

> Please run
> 
> ./mach mochitest
> browser/components/preferences/in-content/tests/browser_siteData.js
> 
> to confirm that the test timeouts for you locally as well.

Turns out the test does NOT fail for me locally. No timeouts. Tried multiple times.

Here's part of the output when I ran the tests:
> Browser Chrome Test Summary
> 	  Passed: 38
>	  Failed: 0
>	  Todo: 0
>	  Mode: e10s
> *** End BrowserChrome Test Results ***



> My fix for this in bug 1422163 was to change site_data_test.html to use this
> line instead:
> 
> tx.oncomplete = () => document.dispatchEvent(new
> CustomEvent("test-indexedDB-done", {bubbles: true, cancelable: false}));
> 
> So you could either do that or, more simply, just don't replace the
> waitForEvent function in browser_siteData.js and don't delete it from
> head.js and leave the removal to me in bug 1422163. I'd accept a patch that
> does either :)

Since the tests did not fail for me locally, please advise on the best course of action.
Flags: needinfo?(jhofmann)
Hm, that's strange, what platform are you on? In any case, it's failing locally for me on OSX and it's failing the Linux try machines, so I'd suggest one of the two options I mentioned in comment 8. Feel free to ask for more directions if anything in that comment wasn't clear :)

Thank you!
Flags: needinfo?(jhofmann)
Pushed by jhofmann@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/44239d2b430a
Use BrowserTestUtils.waitForEvent in preferences tests; r=johannh
(In reply to Johann Hofmann [:johannh] from comment #10)
> Hm, that's strange, what platform are you on? 
Linux. Debian 9.3 to be precise. 

> In any case, it's failing locally for me on OSX and it's failing the 
> Linux try machines, so I'd suggest one of the two options I mentioned 
> in comment 8. Feel free to ask for more directions if anything 
> in that comment wasn't clear :)

What I've decided to do is the following (from comment 8):
> just don't replace the waitForEvent function in browser_siteData.js 
> and don't delete it from head.js and leave the removal to me in bug 1422163.
Flags: needinfo?(jhofmann)
Great, thanks! I did another try run, I'm sure this one will be good for landing it!

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9f69a56db15c0514777ce965d5a589f1ae77afec
Flags: needinfo?(jhofmann)
Pushed by jhofmann@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4d0427742239
Use BrowserTestUtils.waitForEvent in preferences tests; r=johannh
https://hg.mozilla.org/mozilla-central/rev/4d0427742239
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
status-firefox60: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60
You need to log in before you can comment on or make changes to this bug.