Closed Bug 1263784 Opened 4 years ago Closed 4 years ago

Fix test_modal_prompts.html and test_modal_select.html to run under E10S

Categories

(Toolkit :: General, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
e10s + ---
firefox48 --- fixed

People

(Reporter: Dolske, Assigned: Dolske)

References

(Blocks 1 open bug)

Details

Attachments

(5 files)

Followup to bug 1252723, fix test_modal_prompts.html and test_modal_select.html to run under E10S.
Move all actions to be performed on a prompt (e.g. clicking a button, checking a checkbox) into an "action" object.

Review commit: https://reviewboard.mozilla.org/r/45621/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/45621/
Attachment #8740206 - Flags: review?(adw)
Attachment #8740207 - Flags: review?(adw)
Attachment #8740208 - Flags: review?(adw)
Attachment #8740209 - Flags: review?(adw)
Attachment #8740210 - Flags: review?(adw)
Use chromeScript and message manager to fetch state of prompt and dismiss it. Enable tests in E10S.

Review commit: https://reviewboard.mozilla.org/r/45629/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/45629/
A few comments, to hopefully make things easier/clearer:

* This review looks bigger than it really is, a lot of it is just mindless moving things around. :) The basic idea is that the main test is still running in content, and right before triggering a prompt it passes an "action" object to the parent (telling it to start watching for a prompt, and how to interact with / dismiss it). The test already had a "state" object describing what it was expecting in each prompt when shown (title, button labels, etc) -- the parent now gathers a similar object describing what it sees in the actual prompt, and passes that back to the child (which compares that with what it expected).

* The first 3 patches are evolving the test towards cleaner state/action usage.

* The 4th patch looks huge, but should basically be a rubber-stamp review. ("Move state/action objects out of handleDialog, into main runTest() body.") It's just moving all the state/action objects. Previously each subtest was split between runTest (where the API call was setup and returned values checked) and handleDialog (where the expected state/action was checked), which was really kind of confusing. With action/state moved, handleDialog() basically became a 1-line call to checkExpectedState() with the current (global) state/action.

* I'll note reviewboard's diffs look weird for that 4th patch, because it thinks state/action are staying in place and all the code around them is moving. The unified diff might be clearer to look at here, since it more clearly shows just those objects moving.

* The 5th test is where the actual E10Sification happens. chromeScript.js gains functions to gather prompt state and performs the requested actions, the checkPromptState() is altered to check the returned state instead of the UI directly, the main test's startCallbackTimer() is converted to handlePrompt() which messages the parent, and returns a promise that's resolved when the parent returns the prompt state / dismisses the prompt.

I'm tempted to clean up some of the existing tests to use this same general pattern... But not in this bug. :)
Blocks: e10s-tests
tracking-e10s: --- → +
Oops, had a few test failures on not-OSX because I forgot to handle one thing. This simple fix should get them all green, will fold into patches post-first-review.

testfix: https://hg.mozilla.org/try/rev/01a7eb03d01d
Attachment #8740206 - Flags: review?(adw) → review+
Comment on attachment 8740206 [details]
MozReview Request: Bug 1263784 - Fix test_modal_prompts.html and test_modal_select.html to run under E10S. r=adw

https://reviewboard.mozilla.org/r/45621/#review43197
Comment on attachment 8740207 [details]
MozReview Request: Bug 1263784 - Fix test_modal_prompts.html and test_modal_select.html to run under E10S. r=adw

https://reviewboard.mozilla.org/r/45623/#review43199
Attachment #8740207 - Flags: review?(adw) → review+
Attachment #8740208 - Flags: review?(adw) → review+
Comment on attachment 8740208 [details]
MozReview Request: Bug 1263784 - Fix test_modal_prompts.html and test_modal_select.html to run under E10S. r=adw

https://reviewboard.mozilla.org/r/45625/#review43201
Comment on attachment 8740209 [details]
MozReview Request: Bug 1263784 - Fix test_modal_prompts.html and test_modal_select.html to run under E10S. r=adw

https://reviewboard.mozilla.org/r/45627/#review43203
Attachment #8740209 - Flags: review?(adw) → review+
Attachment #8740210 - Flags: review?(adw) → review+
Comment on attachment 8740210 [details]
MozReview Request: Bug 1263784 - Fix test_modal_prompts.html and test_modal_select.html to run under E10S. r=adw

https://reviewboard.mozilla.org/r/45629/#review43205
(In reply to Justin Dolske [:Dolske] from comment #9)
> Oops, had a few test failures on not-OSX because I forgot to handle one
> thing. This simple fix should get them all green, will fold into patches
> post-first-review.
> 
> testfix: https://hg.mozilla.org/try/rev/01a7eb03d01d

This fixed everything, except Linux-E10S is now reporting that the <browser> is sometimes the focused element instead of the expected element in the test.

I suspect this is the same issue as the existing Linux ignore-focus workaround in the test. So I filed bug 1265077 to look into this, and will land this test rewrite in the meantime.
Comment on attachment 8740206 [details]
MozReview Request: Bug 1263784 - Fix test_modal_prompts.html and test_modal_select.html to run under E10S. r=adw

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45621/diff/1-2/
Attachment #8740206 - Attachment description: MozReview Request: Bug 1263784 - Fix test_modal_prompts.html and test_modal_select.html to run under E10S. r?adw → MozReview Request: Bug 1263784 - Fix test_modal_prompts.html and test_modal_select.html to run under E10S. r=adw
Attachment #8740207 - Attachment description: MozReview Request: Bug 1263784 - Fix test_modal_prompts.html and test_modal_select.html to run under E10S. r?adw → MozReview Request: Bug 1263784 - Fix test_modal_prompts.html and test_modal_select.html to run under E10S. r=adw
Attachment #8740208 - Attachment description: MozReview Request: Bug 1263784 - Fix test_modal_prompts.html and test_modal_select.html to run under E10S. r?adw → MozReview Request: Bug 1263784 - Fix test_modal_prompts.html and test_modal_select.html to run under E10S. r=adw
Attachment #8740209 - Attachment description: MozReview Request: Bug 1263784 - Fix test_modal_prompts.html and test_modal_select.html to run under E10S. r?adw → MozReview Request: Bug 1263784 - Fix test_modal_prompts.html and test_modal_select.html to run under E10S. r=adw
Attachment #8740210 - Attachment description: MozReview Request: Bug 1263784 - Fix test_modal_prompts.html and test_modal_select.html to run under E10S. r?adw → MozReview Request: Bug 1263784 - Fix test_modal_prompts.html and test_modal_select.html to run under E10S. r=adw
Comment on attachment 8740207 [details]
MozReview Request: Bug 1263784 - Fix test_modal_prompts.html and test_modal_select.html to run under E10S. r=adw

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45623/diff/1-2/
Comment on attachment 8740208 [details]
MozReview Request: Bug 1263784 - Fix test_modal_prompts.html and test_modal_select.html to run under E10S. r=adw

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45625/diff/1-2/
Comment on attachment 8740209 [details]
MozReview Request: Bug 1263784 - Fix test_modal_prompts.html and test_modal_select.html to run under E10S. r=adw

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45627/diff/1-2/
Comment on attachment 8740210 [details]
MozReview Request: Bug 1263784 - Fix test_modal_prompts.html and test_modal_select.html to run under E10S. r=adw

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45629/diff/1-2/
You need to log in before you can comment on or make changes to this bug.