Closed
Bug 1254592
Opened 8 years ago
Closed 8 years ago
[e10s] fix test_history_post.xul to run in content and remove duped test_bug_94514.html
Categories
(Toolkit :: Places, defect)
Toolkit
Places
Tracking
()
RESOLVED
FIXED
mozilla48
People
(Reporter: mak, Assigned: nhnt11)
References
(Blocks 1 open bug)
Details
Attachments
(1 file, 3 obsolete files)
12.54 KB,
patch
|
nhnt11
:
review+
|
Details | Diff | Splinter Review |
the test should really run in content, probably can be converted to a mochitest-plain or mochitest-browser.
Updated•8 years ago
|
tracking-e10s:
--- → +
Reporter | ||
Comment 1•8 years ago
|
||
I think we should go with a mochitest-plain with loadChromeScript (bug 1251139 is about to land) the test should submit the form, wait for the iframe to be loaded, then wait for the chrome script to check the Places database.
Reporter | ||
Comment 2•8 years ago
|
||
while here, we should also remove test_bug_94514.html and bug94514-postpage.html, since they are testing the same thing...
Summary: [e10s] test_history_post.xul should run in content → [e10s] fix test_history_post.xul to run in content and remove duped test_bug_94514.html
Reporter | ||
Comment 3•8 years ago
|
||
In the end, this would end up being the only mochitest-plain in Places... I'd to limit the harnesses we can fail so let's go with mochitest-browser instead.
Reporter | ||
Comment 4•8 years ago
|
||
Sorry, I mistyped most of the comment. The point is this test should be rewritten as a mochitest-browser.
Reporter | ||
Comment 6•8 years ago
|
||
Feel free to take it and thank you. I didn't start working on it, apart from looking if it was needed to convert it for e10s.
Assignee | ||
Comment 7•8 years ago
|
||
Attachment #8730517 -
Flags: review?(mak77)
Assignee | ||
Comment 8•8 years ago
|
||
Rebased.
Attachment #8730517 -
Attachment is obsolete: true
Attachment #8730517 -
Flags: review?(mak77)
Attachment #8730521 -
Flags: review?(mak77)
Reporter | ||
Comment 9•8 years ago
|
||
Comment on attachment 8730521 [details] [diff] [review] Patch Review of attachment 8730521 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, still something to fix, will have a quick second look. ::: toolkit/components/places/tests/browser/browser_history_post.js @@ +1,1 @@ > +Cu.import("resource://gre/modules/PlacesUtils.jsm"); this should not be needed case head.js does that already @@ +3,5 @@ > +const PAGE_URI = "http://example.com/tests/toolkit/components/places/tests/browser/history_post.html"; > +const SJS_URI = NetUtil.newURI("http://example.com/tests/toolkit/components/places/tests/browser/history_post.sjs"); > + > +add_task(function* () { > + yield BrowserTestUtils.withNewTab({gBrowser: gBrowser, url: PAGE_URI}, Task.async(function* (aBrowser) { I think you can avoid passing gBrowser, if no browser is specified it will default to gBrowser. @@ +18,5 @@ > + submit.click(); > + yield p; > + }); > + let history = Cc["@mozilla.org/browser/history;1"] > + .getService(Ci.mozIAsyncHistory); This is not necessary, just use PlacesUtils.history where needed. @@ +20,5 @@ > + }); > + let history = Cc["@mozilla.org/browser/history;1"] > + .getService(Ci.mozIAsyncHistory); > + yield new Promise((resolve, reject) => { > + history.isURIVisited(SJS_URI, function (aURI, aIsVisited) { you can use promiseIsURIVisited from head.js @@ +24,5 @@ > + history.isURIVisited(SJS_URI, function (aURI, aIsVisited) { > + ok(!aIsVisited, "The POST page should not be added to history"); > + > + let db = PlacesUtils.history.QueryInterface(Ci.nsPIPlacesDatabase) > + .DBConnection; Please let's use the async API. let db = yield PlacesUtils.promiseDBConnection(); let rows = yield db.execute(query, { page_url: SJS_URI.spec }); ok(rows.length, 0, ...)
Attachment #8730521 -
Flags: review?(mak77)
Assignee | ||
Comment 10•8 years ago
|
||
Thanks! (In reply to Marco Bonardo [::mak] from comment #9) > @@ +3,5 @@ > > +const PAGE_URI = "http://example.com/tests/toolkit/components/places/tests/browser/history_post.html"; > > +const SJS_URI = NetUtil.newURI("http://example.com/tests/toolkit/components/places/tests/browser/history_post.sjs"); > > + > > +add_task(function* () { > > + yield BrowserTestUtils.withNewTab({gBrowser: gBrowser, url: PAGE_URI}, Task.async(function* (aBrowser) { > > I think you can avoid passing gBrowser, if no browser is specified it will > default to gBrowser. Following the code path from [1], I don't think so... [1] https://dxr.mozilla.org/mozilla-central/rev/d6ee82b9a74155b6bfd544166f036fc572ae8c56/testing/mochitest/BrowserTestUtils/BrowserTestUtils.jsm#65
Attachment #8730521 -
Attachment is obsolete: true
Attachment #8730820 -
Flags: review?(mak77)
Reporter | ||
Comment 11•8 years ago
|
||
(In reply to Nihanth Subramanya [:nhnt11] from comment #10) > Following the code path from [1], I don't think so... > [1] > https://dxr.mozilla.org/mozilla-central/rev/ > d6ee82b9a74155b6bfd544166f036fc572ae8c56/testing/mochitest/BrowserTestUtils/ > BrowserTestUtils.jsm#65 ah, you're right, that code doesn't make sense imo, but ok :)
Reporter | ||
Comment 12•8 years ago
|
||
Comment on attachment 8730820 [details] [diff] [review] Patch v2 Review of attachment 8730820 [details] [diff] [review]: ----------------------------------------------------------------- thanks, lgtm. ::: toolkit/components/places/tests/browser/browser_history_post.js @@ +1,5 @@ > +const PAGE_URI = "http://example.com/tests/toolkit/components/places/tests/browser/history_post.html"; > +const SJS_URI = NetUtil.newURI("http://example.com/tests/toolkit/components/places/tests/browser/history_post.sjs"); > + > +add_task(function* () { > + yield BrowserTestUtils.withNewTab({gBrowser: gBrowser, url: PAGE_URI}, Task.async(function* (aBrowser) { nit: just gBrowser (in ES6 when defining an object you can just type {gBrowser} and it will be the same as {gBrowser: gBrowser}) ::: toolkit/components/places/tests/browser/history_post.html @@ +9,5 @@ > + <input type="submit" id="submit"/> > + </form> > + <p id="display"></p> > + <div id="content" style="display:none;"></div> > + <pre id="test"></pre> nit: The <p>, <div> and <pre> are likely pointless in a mochitest-browser test since the harness doesn't use them.
Attachment #8730820 -
Flags: review?(mak77) → review+
Assignee | ||
Comment 13•8 years ago
|
||
Attachment #8730820 -
Attachment is obsolete: true
Attachment #8731477 -
Flags: review+
Assignee | ||
Updated•8 years ago
|
Depends on: 1255066
Keywords: checkin-needed
Comment 14•8 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/632931aedcbb
Keywords: checkin-needed
Comment 15•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/632931aedcbb
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in
before you can comment on or make changes to this bug.
Description
•