Closed Bug 1261306 Opened 8 years ago Closed 8 years ago

Convert test_autocomplete_change_after_focus.html to mochitest-plain for e10s coverage

Categories

(Toolkit :: Autocomplete, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
e10s + ---
firefox49 --- fixed

People

(Reporter: mikedeboer, Assigned: mikedeboer)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

See lengthy summary.
Flags: qe-verify-
Flags: firefox-backlog+
Summary: Convert test_autocomplete_change_after_focus.html test_autocomplete_with_composition_on_input.html to mochitest-browser tests for e10s coverage → Convert test_autocomplete_change_after_focus.html to mochitest-browser tests for e10s coverage
Attachment #8738132 - Flags: review?(MattN+bmo)
Component: Form Manager → Password Manager
Summary: Convert test_autocomplete_change_after_focus.html to mochitest-browser tests for e10s coverage → Convert test_autocomplete_change_after_focus.html to a mochitest-browser test for e10s coverage
Comment on attachment 8738132 [details] [diff] [review]
Convert test to a mochitest-browser one

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

By moving this to b-c we lose test coverage outside of Fx (e.g. Thunderbird). It would be simpler and less of a change to just wrap the body of `setup` in an anonymous function passed to loadChromeScript then have handleCompletion send a message back to the child to call runTest.
Attachment #8738132 - Flags: review?(MattN+bmo)
(In reply to Matthew N. [:MattN] from comment #2)
> By moving this to b-c we lose test coverage outside of Fx (e.g.
> Thunderbird). It would be simpler and less of a change to just wrap the body
> of `setup` in an anonymous function passed to loadChromeScript then have
> handleCompletion send a message back to the child to call runTest.

Sorry for the late reply, but is the goal of this bug now that the test can be run without hitting CPOW usage errors, or am I supposed to make sure that this test can also be run in a remote iframe?
This particular test is not disabled atm, so that part is covered, but how does your instruction above add e10s coverage?
I can only see calling `loadChromeScript` on the global message manager here, but I'm not sure what that would solve.
Is there another example in-tree?
Flags: needinfo?(MattN+bmo)
Apologies for the late reply too.

(In reply to Mike de Boer [:mikedeboer] from comment #3)
> (In reply to Matthew N. [:MattN] from comment #2)
> > By moving this to b-c we lose test coverage outside of Fx (e.g.
> > Thunderbird). It would be simpler and less of a change to just wrap the body
> > of `setup` in an anonymous function passed to loadChromeScript then have
> > handleCompletion send a message back to the child to call runTest.
> 
> Sorry for the late reply, but is the goal of this bug now that the test can
> be run without hitting CPOW usage errors, or am I supposed to make sure that
> this test can also be run in a remote iframe?

Maybe the former but perhaps closer to the latter (I think I'm confused by your choices) but you don't have to deal with iframes yourself as mochitest-plain already runs in a remote browser when running with e10s. The problem with mochitest-chrome is that even when the browser has e10s enabled, the test harness tab is loaded in a non-remote browser. The problem is less about CPOWs for this test and more about not accessing form history from the content process. You need to run the the form history in the parent process and the easiest way to do that from mochitest-plain is with loadChromeScript (see below).

> This particular test is not disabled atm, so that part is covered, but how
> does your instruction above add e10s coverage?

I guess I forgot to explicitly say that it should be moved to mochitest-plain instead. That's how we get e10s coverage.

> I can only see calling `loadChromeScript` on the global message manager
> here, but I'm not sure what that would solve.
> Is there another example in-tree?

See http://hg.mozilla.org/mozilla-central/rev/d823452bea13 where I converted another m-c to m-p. `runInParent` there is a wrapper for `loadChromeScript`: https://mxr.mozilla.org/mozilla-central/source/toolkit/components/passwordmgr/test/pwmgr_common.js?rev=ea109c3f6b3c&mark=313#306
Flags: needinfo?(MattN+bmo)
Matt, thanks so much for your explanation - wouldn't have gotten far without it ;-)
Even though this might seem like a complete rewrite, most of the changes are due to 
1) converting to `add_task`
2) jumping from content - chrome - content.
Attachment #8738132 - Attachment is obsolete: true
Attachment #8742725 - Flags: review?(MattN+bmo)
Comment on attachment 8742725 [details] [diff] [review]
Patch v2: convert test_autocomplete_change_after_focus.html from m-chrome to m-plain

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

Thanks Mike, moving this over to Dolske so maybe I can have time to work on some patches of my own one of these days. I've been getting ~6 reviews/feedback per day…
Attachment #8742725 - Flags: review?(MattN+bmo) → review?(dolske)
Component: Password Manager → Autocomplete
Summary: Convert test_autocomplete_change_after_focus.html to a mochitest-browser test for e10s coverage → Convert test_autocomplete_change_after_focus.html to mochitest-plain for e10s coverage
Comment on attachment 8742725 [details] [diff] [review]
Patch v2: convert test_autocomplete_change_after_focus.html from m-chrome to m-plain

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

LGTM.
Attachment #8742725 - Flags: review?(dolske) → review+
https://hg.mozilla.org/integration/fx-team/rev/9ad67a907cae9644493ecbabae4529602ca3fa72
Bug 1261306 - convert test_autocomplete_change_after_focus.html from m-chrome to m-plain. r=dolske
backed out for test failures like  https://treeherder.mozilla.org/logviewer.html#?job_id=8865042&repo=fx-team
Flags: needinfo?(mdeboer)
https://hg.mozilla.org/integration/fx-team/rev/7353d8e2cef652857f730e3b9ce39425e388e832
Bug 1261306 - convert test_autocomplete_change_after_focus.html from m-chrome to m-plain. r=dolske
Flags: needinfo?(mdeboer)
https://hg.mozilla.org/mozilla-central/rev/7353d8e2cef6
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
You need to log in before you can comment on or make changes to this bug.