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)
Toolkit
Autocomplete
Tracking
()
RESOLVED
FIXED
mozilla49
People
(Reporter: mikedeboer, Assigned: mikedeboer)
References
(Blocks 1 open bug)
Details
Attachments
(1 file, 1 obsolete file)
7.03 KB,
patch
|
Dolske
:
review+
|
Details | Diff | Splinter Review |
See lengthy summary.
Flags: qe-verify-
Flags: firefox-backlog+
Updated•8 years ago
|
Blocks: e10s-tests
Assignee | ||
Updated•8 years ago
|
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
Assignee | ||
Comment 1•8 years ago
|
||
Attachment #8738132 -
Flags: review?(MattN+bmo)
Assignee | ||
Updated•8 years ago
|
Component: Form Manager → Password Manager
Assignee | ||
Updated•8 years ago
|
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 2•8 years ago
|
||
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)
Assignee | ||
Comment 3•8 years ago
|
||
(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)
Comment 4•8 years ago
|
||
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)
Assignee | ||
Comment 5•8 years ago
|
||
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 6•8 years ago
|
||
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)
Updated•8 years ago
|
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 7•8 years ago
|
||
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+
Assignee | ||
Comment 8•8 years ago
|
||
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
Comment 9•8 years ago
|
||
backed out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=8865042&repo=fx-team
Flags: needinfo?(mdeboer)
Comment 10•8 years ago
|
||
Backout: https://hg.mozilla.org/integration/fx-team/rev/4ac3b4265ce2
Assignee | ||
Comment 11•8 years ago
|
||
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
Assignee | ||
Updated•8 years ago
|
Flags: needinfo?(mdeboer)
Comment 12•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/7353d8e2cef6
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox49:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
You need to log in
before you can comment on or make changes to this bug.
Description
•