Use BrowserTestUtils.waitForEvent in preferences tests

RESOLVED FIXED in Firefox 60

Status

()

enhancement
P5
normal
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: johannh, Assigned: arjunkrishnababu96, Mentored)

Tracking

58 Branch
Firefox 60
Points:
---

Firefox Tracking Flags

(firefox60 fixed)

Details

Attachments

(2 attachments)

Reporter

Description

a year ago
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
Assignee

Comment 1

a year ago
I'd like to work on this. Could you please point me in the right direction?
Thanks!
Flags: needinfo?(jhofmann)
Reporter

Comment 2

a year ago
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)
Assignee

Comment 3

a year ago
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)
Reporter

Comment 4

a year ago
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 hidden (mozreview-request)
Reporter

Comment 6

a year ago
mozreview-review
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+
Assignee

Comment 7

a year ago
I'm a newbie and I'm asking out of curiosity: what happens now?
Reporter

Comment 8

a year ago
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!
Assignee

Comment 9

a year ago
(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)
Reporter

Comment 10

a year ago
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)

Comment 11

a year ago
Pushed by jhofmann@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/44239d2b430a
Use BrowserTestUtils.waitForEvent in preferences tests; r=johannh
Assignee

Comment 13

a year ago
(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.
Comment hidden (mozreview-request)
Assignee

Updated

a year ago
Flags: needinfo?(jhofmann)
Reporter

Comment 15

a year ago
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)

Comment 16

a year ago
Pushed by jhofmann@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4d0427742239
Use BrowserTestUtils.waitForEvent in preferences tests; r=johannh

Comment 17

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/4d0427742239
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60
You need to log in before you can comment on or make changes to this bug.