1428845 - Use TestUtils.waitForCondition in preferences tests

Status: RESOLVED FIXED

(Firefox :: Settings UI, enhancement, P5)

Description

[Johann Hofmann [:johannh]]

There's a waitForCondition function in preferences/in-content/tests/head.js[0] which is not needed because we can simply use TestUtils.waitForCondition[1] instead.

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

[0] https://searchfox.org/mozilla-central/rev/f42618c99dcb522fb674221acfbc68c2d92e7936/browser/components/preferences/in-content/tests/head.js#145
[1] https://searchfox.org/mozilla-central/rev/f42618c99dcb522fb674221acfbc68c2d92e7936/testing/modules/TestUtils.jsm#116
[2] https://searchfox.org/mozilla-central/search?q=%5Cswaitforcondition&case=false&regexp=true&path=browser%2Fcomponents%2Fpreferences%2Fin-content%2Ftests

Comment 1

[Rofael Aleezada [:rofael]]

Attachment: Bug 1428845 - Removed duplicate definition of waitForCondition().

Review commit: https://reviewboard.mozilla.org/r/214758/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/214758/

Comment 2

[Rofael Aleezada [:rofael]]

I removed [0] and added "TestUtils." to everything in [2]. I ran all the modified tests and didn't get any syntax errors.

Comment 3

[Johann Hofmann [:johannh]]

Comment on attachment 8944595 [details]
Bug 1428845 - Removed duplicate definition of waitForCondition().

https://reviewboard.mozilla.org/r/214758/#review220472

That looks good to me! Thank you! Let's give it a quick try run and land it when that succeeds!

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4d03b6d9fff4
Removed duplicate definition of waitForCondition(). r=johannh

Comment 5

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/be344306e4bb
Backed out changeset 4d03b6d9fff4 for failing browser chrome on  browser/components/preferences/in-content/tests/browser_applications_selection.js

Comment 6

[Rofael Aleezada [:rofael]]

Oops...

https://dxr.mozilla.org/mozilla-central/source/browser/components/preferences/in-content/tests/browser_applications_selection.js#36 and https://dxr.mozilla.org/mozilla-central/source/browser/components/preferences/in-content/tests/browser_applications_selection.js#51 try to load the actionsMenu into a var, but it returns null!

Turns out that TestUtils.waitForCondition doesn't resolve anything (https://searchfox.org/mozilla-central/source/testing/modules/TestUtils.jsm#139), whereas the one in head.js (https://searchfox.org/mozilla-central/source/browser/components/preferences/in-content/tests/head.js#151) resolves with the output of the function passed into. 

TestUtils.wfc does run the function passed into it and stores the result in conditionPassed. I'm going to try resolve(conditionPassed) and see if that passes the tests (and doesn't break anything else)

Comment 7

[Rofael Aleezada [:rofael]]

That was exactly the problem! I'm running all of browser/components/preferences/in-content/tests/ right now to check that it didn't unintentionally break something else. I'll push it to review as soon as that's dealt with.

Comment 8

[Rofael Aleezada [:rofael]]

Comment on attachment 8944595 [details]
Bug 1428845 - Removed duplicate definition of waitForCondition().

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/214758/diff/1-2/

Comment 9

[Johann Hofmann [:johannh]]

Comment on attachment 8944595 [details]
Bug 1428845 - Removed duplicate definition of waitForCondition().

https://reviewboard.mozilla.org/r/214758/#review220794

Ah, yeah, that seems like a logical improvement to me, let's see whether it breaks anything. I wonder why the try run didn't fail, though...

In any case, it seems like you'd benefit from having access to the Try server yourself. This page shows you how to get level 1 access: https://wiki.mozilla.org/ReleaseEngineering/TryServer#Getting_access_to_the_Try_Server. I'm happy to vouch for you.

Great work!

::: testing/modules/TestUtils.jsm:113
(Diff revision 2)
>     * @param attempts
>     *        The number of times to poll before giving up and rejecting
>     *        if the condition has not yet returned true. Defaults to 50
>     *        (~5 seconds for 100ms intervals)
>     * @return Promise
>     *        Resolves when condition is true.

Please add a note that it resolves with the return value of the condition function here.

Comment 10

[Johann Hofmann [:johannh]]

Previous try run was here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d19409d5d292
New try run here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f578eeda4dbb58b79fba70b11613ec156a740f46

Comment 11

[Johann Hofmann [:johannh]]

Try didn't fail because of https://dxr.mozilla.org/mozilla-central/rev/c4ebc8c28a33b785dfbfa533810517cc707d1ad0/browser/components/preferences/in-content/tests/browser.ini#14

*sigh*

Comment 12

[Rofael Aleezada [:rofael]]

Comment on attachment 8944595 [details]
Bug 1428845 - Removed duplicate definition of waitForCondition().

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/214758/diff/2-3/

Comment 13

[Rofael Aleezada [:rofael]]

(In reply to Johann Hofmann [:johannh] from comment #9)

Okay, I just pushed that change to review. I also made a request for level 1 access (bug 1432957).

Thanks for the vouch!

Comment 14

[Johann Hofmann [:johannh]]

Great, let's get this landed!

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ef78d4e91a01
Removed duplicate definition of waitForCondition(). r=johannh

Comment 16

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/ef78d4e91a01