Right now we only have `is` and `isnot` and `ok` to do assertions. It's two lines of code to add Assert.jsm in there and you get a whole lot more methods at our devs' disposal. (throws, rejects, etc).

Built on top of the patch in bug 1241532.

Comment on attachment 8711052 [details] [diff] [review] Patch v1: make all Assert.jsm assert methods available for use in ContentTasks Review of attachment 8711052 [details] [diff] [review]: ----------------------------------------------------------------- This looks good to me. I wonder if we should take advantage of the few consumers of ContentTask (for now), and deprecate the usage of ok, is and isnot from within it, in preference to Assert.jsm?

(In reply to Mike Conley (:mconley) - Needinfo me! from comment #2) > I wonder if we should take advantage of the few consumers of ContentTask > (for now), and deprecate the usage of ok, is and isnot from within it, in > preference to Assert.jsm? We could, but isn't that going to tick people off? I mean, there were _very_ strong opinions about unifying everything to Assert.*, so I can imagine that to be the case here... Apart from that, I'd be happy to do that of course! (I'll put up a patch for that for posterity).

Comment on attachment 8712121 [details] [diff] [review] Patch 2: convert all assertion methods used inside content tasks to the Assert.* family Review of attachment 8712121 [details] [diff] [review]: ----------------------------------------------------------------- ::: testing/mochitest/BrowserTestUtils/content/content-task.js @@ +34,4 @@ > > + var ok = Assert.ok.bind(Assert); > + var is = Assert.equal.bind(Assert); > + var isnot = Assert.notEqual.bind(Assert); We can move this to patch v1, if you think it looks better. I do like the fact that we're unifying the implementations, so it might be better to have regardless of the conversions. ::: toolkit/components/passwordmgr/test/browser/browser_DOMFormHasPassword.js @@ +88,5 @@ > "<form id='" + ids.FORM2_ID + "'></form>" + > "</body></html>"); > yield promise; > > + Assert.ok(true, "Test completed"); This assertion looks like a pleonasm to me :) I'd be happy to remove it.

Comment on attachment 8712121 [details] [diff] [review] Patch 2: convert all assertion methods used inside content tasks to the Assert.* family Review of attachment 8712121 [details] [diff] [review]: ----------------------------------------------------------------- So I like the idea of using Assert.jsm and friends inside ContentTask. There are a lot of older, cruftier tests that will use the old ok / is, and that's fine, but the number of tests with ContentTasks is small enough that we can really make an inroad here. I'm noticing, however, that a good chunk of this patch brings in Assert.jsm for non-ContentTask comparisons. I think we shouldn't have those in this patch. That's where my feedback- is coming from. ::: browser/base/content/test/general/browser_clipboard.js @@ -124,3 @@ > }); > > - is(results.length, 15, "Correct number of results"); Lovely to get rid of this hackery. ::: browser/components/sessionstore/test/browser_multiple_navigateAndRestore.js @@ +9,5 @@ > forceNotRemote: true, > }); > gBrowser.selectedTab = tab; > let browser = gBrowser.selectedBrowser; > + Assert.ok(!browser.isRemoteBrowser, "Ensure browser is not remote"); This isn't inside a ContentTask. @@ +23,5 @@ > let state = JSON.parse(ss.getTabState(tab)); > let entries = state.entries; > + Assert.equal(entries.length, 1, "There should only be one entry"); > + Assert.equal(entries[0].url, PAGE_2, "Should have PAGE_2 as the sole history entry"); > + Assert.equal(browser.currentURI.spec, PAGE_2, "Should have PAGE_2 as the browser currentURI"); The above are not in a ContentTask. ::: browser/extensions/pdfjs/test/browser_pdfjs_main.js @@ +10,5 @@ > let handlerInfo = mimeService.getFromTypeAndExtension('application/pdf', 'pdf'); > > // Make sure pdf.js is the default handler. > + Assert.equal(handlerInfo.alwaysAskBeforeHandling, false, 'pdf handler defaults to always-ask is false'); > + Assert.equal(handlerInfo.preferredAction, Ci.nsIHandlerInfo.handleInternally, 'pdf handler defaults to internal'); These aren't inside a ContentTask. @@ +18,5 @@ > yield BrowserTestUtils.withNewTab({ gBrowser: gBrowser, url: "about:blank" }, > function* (newTabBrowser) { > yield waitForPdfJS(newTabBrowser, TESTROOT + "file_pdfjs_test.pdf"); > > + Assert.ok(gBrowser.isFindBarInitialized(), "Browser FindBar initialized!"); This isn't inside a ContentTask. ::: browser/extensions/pdfjs/test/browser_pdfjs_navigation.js @@ +254,5 @@ > }); > > // Get the element and trigger the action for changing the page > var el = document.querySelector(test.action.selector); > + Assert.ok(el, "Element '" + test.action.selector + "' has been found"); This isn't inside a ContentTask. ::: browser/extensions/pdfjs/test/browser_pdfjs_views.js @@ +10,5 @@ > let handlerInfo = mimeService.getFromTypeAndExtension('application/pdf', 'pdf'); > > // Make sure pdf.js is the default handler. > + Assert.equal(handlerInfo.alwaysAskBeforeHandling, false, 'pdf handler defaults to always-ask is false'); > + Assert.equal(handlerInfo.preferredAction, Ci.nsIHandlerInfo.handleInternally, 'pdf handler defaults to internal'); These aren't inside a ContentTask. ::: browser/extensions/pdfjs/test/browser_pdfjs_zoom.js @@ +66,5 @@ > > // Make sure pdf.js is the default handler. > + Assert.equal(handlerInfo.alwaysAskBeforeHandling, false, > + 'pdf handler defaults to always-ask is false'); > + Assert.equal(handlerInfo.preferredAction, Ci.nsIHandlerInfo.handleInternally, These aren't inside a ContentTask. ::: docshell/test/browser/browser_bug234628-1.js @@ +3,5 @@ > runCharsetTest(rootDir + "file_bug234628-1.html", afterOpen, "windows-1251", afterChangeCharset); > } > > function afterOpen() { > + Assert.equal(content.document.documentElement.textContent.indexOf('\u20AC'), 129, "Parent doc should be windows-1252 initially"); None of these are running in a ContentTask. ::: docshell/test/browser/browser_bug234628-10.js @@ +3,5 @@ > runCharsetTest(rootDir + "file_bug234628-10.html", afterOpen, "windows-1251", afterChangeCharset); > } > > function afterOpen() { > + Assert.equal(content.document.documentElement.textContent.indexOf('\u20AC'), 151, "Parent doc should be windows-1252 initially"); None of these are in a ContentTask. ::: docshell/test/browser/browser_bug234628-11.js @@ +3,5 @@ > runCharsetTest(rootDir + "file_bug234628-11.html", afterOpen, "windows-1251", afterChangeCharset); > } > > function afterOpen() { > + Assert.equal(content.document.documentElement.textContent.indexOf('\u20AC'), 193, "Parent doc should be windows-1252 initially"); None of these are in a ContentTask. ::: testing/mochitest/BrowserTestUtils/content/content-task.js @@ +26,5 @@ > sendAsyncMessage("content-task:test-result", { > + id: id, > + condition: !err, > + name: err ? err.message : message, > + diag: "", Is diag even needed? It looks like it's eventually used on the parent side here: https://dxr.mozilla.org/mozilla-central/source/testing/mochitest/browser-test.js#920 This, I guess, tells us where the failure occurred. I guess we're already getting that with the stack? Or is there a good reason to keep diag around? @@ +34,4 @@ > > + var ok = Assert.ok.bind(Assert); > + var is = Assert.equal.bind(Assert); > + var isnot = Assert.notEqual.bind(Assert); I guess I'm fine with this. Folks can use these if they're used to it, but Assert.jsm and friends will be around for the more powerful comparisons.

Re-requesting review, because I put the changes in patch 2 in here and removed `diag`, as requested.

Well, hum, that this list grew this large is entirely my bad for not finishing this up sooner. We are copy-'n-paste monsters! I've made some micro-improvements to ContentTask usage here and there, for which I hope you'll forgive me, and some changes are just 'OMG how did this every work?'-style changes. Please don't be overwhelmed by the sheer amount of file... splinter lets you neatly tick off the files you're done with.

Jim, I was going over this whilst working on this bug, but it seems like this is a bug: https://hg.mozilla.org/mozilla-central/diff/103700d6b355/browser/base/content/test/plugins/browser_blocking.js#l1.354 Which you introduced in bug 1129040: the return value of ContentTask.spawn is _always_ true-ish, because it returns a Promise. I think you forgot to put a `yield` there. However, the try run in comment 10 seems to suggest that you expect `result` to be `true`, but it's not. Is it safe for me to turn the assertion around to expect `false`?

Comment on attachment 8724132 [details] [diff] [review] Patch 1.1: make all Assert.jsm assert methods available for use in ContentTasks Review of attachment 8724132 [details] [diff] [review]: ----------------------------------------------------------------- ::: testing/mochitest/tests/browser/browser_BrowserTestUtils.js @@ +16,5 @@ > let browser = tab.linkedBrowser; > yield BrowserTestUtils.synthesizeMouseAtCenter("#one", {}, browser); > let details = yield getLastEventDetails(browser); > > + Assert.equal(details, "button,31,34", "synthesizeMouseAtCenter"); None of these are inside a ContentTask.

Ah sorry, forgot to remove these! I took care not to touch non-ContentTask assertions in the other tests, though...

Comment on attachment 8724376 [details] [diff] [review] Patch 2.2: convert browser tests Review of attachment 8724376 [details] [diff] [review]: ----------------------------------------------------------------- r-'ing while I wait for an explanation for the sign-flipping in browser_blocking.js. Otherwise, this looks fine. ::: browser/base/content/test/general/browser_PageMetaData_pushstate.js @@ +17,1 @@ > content.history.pushState({}, "2", "2.html"); Might as well combine this with the ContentTask above. @@ +24,4 @@ > > + is(gBrowser.currentURI.spec, rooturi + "2.html", "gBrowser has correct url"); > + yield ContentTask.spawn(gBrowser.selectedBrowser, { rooturi }, function* (args) { > + Assert.equal(content.document.documentURI, args.rooturi + "2.html", If you put this one above, you can combine it with the other two ContentTasks, and then just do the gBrowser.currentURI.spec comparison afterwards. ::: browser/base/content/test/general/browser_bug561636.js @@ +72,5 @@ > } > > function* checkChildFocus(browser, message) > { > + message.testId = testId; Instead of augmenting a pre-existing object, can you pass a different property in the ContentTask.spawn second argument for the test ID? ::: browser/base/content/test/general/browser_clipboard.js @@ +22,5 @@ > // The values are from widget/windows/nsDataObj.cpp. > const htmlPrefix = (navigator.platform.indexOf("Win") >= 0) ? "<html><body>\n<!--StartFragment-->" : ""; > const htmlPostfix = (navigator.platform.indexOf("Win") >= 0) ? "<!--EndFragment-->\n</body>\n</html>" : ""; > > + yield ContentTask.spawn(browser, { modifier: modifier, htmlPrefix: htmlPrefix, htmlPostfix: htmlPostfix }, While you're here, ES6 should let us just do: { modifier, htmlPrefix, htmlPostfix } @@ +131,5 @@ > > // Focus the content again > yield SimpleTest.promiseFocus(browser.contentWindowAsCPOW); > > + yield ContentTask.spawn(browser, { modifier: modifier, htmlPrefix: htmlPrefix, htmlPostfix: htmlPostfix }, Same as above, re: { modifier, htmlPrefix, ... } ::: browser/base/content/test/general/browser_visibleFindSelection.js @@ +25,5 @@ > scrollPromise = promiseWaitForEvent(gBrowser, "scroll"); > EventUtils.synthesizeKey("g", { accelKey: true }); > yield scrollPromise; > > + yield ContentTask.spawn(gBrowser.selectedBrowser, { }, function* (arg) { Might as well just pass null as the second arg to spawn, and don't accept the arg in the function. ::: browser/base/content/test/plugins/browser_blocking.js @@ +93,2 @@ > > + yield ContentTask.spawn(gTestBrowser, null, function* () { Might as well merge this into the ContentTask above. @@ +346,3 @@ > let plugin = content.document.getElementById("test"); > let objLoadingContent = plugin.QueryInterface(Ci.nsIObjectLoadingContent); > + Assert.ok(!objLoadingContent.activated, "Plugin should NOT be activated."); You've flipped the sign here, and the message. Has this been broken on CI, or have you somehow fixed a test bug here, or what? ::: browser/base/content/test/plugins/browser_bug743421.js @@ +113,5 @@ > content.history.replaceState({}, "", "replacedState"); > new XPCNativeWrapper(XPCNativeWrapper.unwrap(content).addPlugin()); > + plugin = content.document.getElementsByTagName("embed")[3]; > + objLoadingContent = plugin.QueryInterface(Ci.nsIObjectLoadingContent); > + Assert.ok(objLoadingContent.activated, "Test 1f, Plugin should not be activated"); Huh... is this testing the right thing? Apparently, "Plugin should not be activated", and yet here it is, being tested as activated, and probably passing on our CI. Can you please file a follow-up to investigate this? Either the string is wrong, or we're busted. @@ +114,5 @@ > new XPCNativeWrapper(XPCNativeWrapper.unwrap(content).addPlugin()); > + plugin = content.document.getElementsByTagName("embed")[3]; > + objLoadingContent = plugin.QueryInterface(Ci.nsIObjectLoadingContent); > + Assert.ok(objLoadingContent.activated, "Test 1f, Plugin should not be activated"); > + }); Trailing ws ::: browser/base/content/test/plugins/browser_pluginCrashReportNonDeterminism.js @@ +176,5 @@ > cancelable: true, > }); > > plugin.dispatchEvent(event); > + Assert.ok(statusDiv.getAttribute("status") == "please", Assert.equal? @@ +225,5 @@ > }); > > plugin.dispatchEvent(event); > > + Assert.ok(statusDiv.getAttribute("status") != "please", Assert.notEqual? @@ +245,5 @@ > let statusDiv = plugin.ownerDocument > .getAnonymousElementByAttribute(plugin, "anonid", > "submitStatus"); > > + Assert.ok(statusDiv.getAttribute("status") == "please", Assert.equal? ::: browser/base/content/test/plugins/browser_plugin_reloading.js @@ +69,5 @@ > let plugin = content.document.getElementById("test"); > let npobj1 = Components.utils.waiveXrays(plugin).getObjectValue(); > plugin.src = plugin.src; > let pluginsDiffer = false; > try { Might as well fixed the busted indentation from lines 73-79 while you're here. ::: browser/components/preferences/in-content/tests/browser_defaultbrowser_alwayscheck.js @@ +12,2 @@ > let alwaysCheck = content.document.getElementById("alwaysCheckDefault"); > + Assert.equal(alwaysCheck.checked, false, "Always Check is unchecked by default"); A bunch of these compares against true / false could be done with Assert.ok, but I won't harp on it. ::: browser/components/privatebrowsing/test/browser/browser_privatebrowsing_aboutHomeButtonAfterWindowClose.js @@ +13,5 @@ > let browser = tab.linkedBrowser; > > yield BrowserTestUtils.browserLoaded(browser); > > + yield ContentTask.spawn(browser, null, function* (){ Space before { ::: browser/components/privatebrowsing/test/browser/browser_privatebrowsing_aboutSessionRestore.js @@ +13,5 @@ > let browser = tab.linkedBrowser; > > yield BrowserTestUtils.browserLoaded(browser); > > + yield ContentTask.spawn(browser, null, function* (){ Space before { ::: browser/components/sessionstore/test/browser_463206.js @@ +44,5 @@ > + //Assert.equal(content.frames[0].frames[1].document.getElementById("in1").value, > + // "", "id prefixes aren't mixed up"); > + Assert.equal(content.frames[1].frames[0].document.getElementById("in1").value, > + "", "id prefixes aren't mixed up"); > + }); Nit: trailing ws ::: browser/components/sessionstore/test/browser_sessionStoreContainer.js @@ +1,5 @@ > /* This Source Code Form is subject to the terms of the Mozilla Public > * License, v. 2.0. If a copy of the MPL was not distributed with this > * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > > function retrieveUserContextId(browser) { This should just be removed?

These are big-ass patches. I'll hopefully do one of these a day for the next few days.

Thanks so much, Mike! Patch 2.2 is the humongous one, I'm afraid - the others are way smaller (a tenth of 2.2 on average). I *think* you can do the other patches in a day or two combined with your own busy work schedule. Your comments re. the plugins tests are the cause of my n-i for Jim.

Comment on attachment 8724377 [details] [diff] [review] Patch 3: convert devtools tests Review of attachment 8724377 [details] [diff] [review]: ----------------------------------------------------------------- ::: devtools/client/commandline/test/browser_cmd_screenshot.js @@ +151,5 @@ > }, > post: Task.async(function*() { > let imgSize = yield getImageSizeFromClipboard(); > + yield ContentTask.spawn(browser, imgSize, function*(imgSize) { > + Assert.equal(imgSize.width, content.innerWidth + content.scrollMaxX - The way this is indented makes it a bit harder to read the arithmetic. Maybe format these like: ```JavaScript Assert.equal(imgSize.width, content.innerWidth + content.scrollMaxX - content.scrollMinX, "Image width matches page size"); //etc ```

Comment on attachment 8724379 [details] [diff] [review] Patch 5: convert dom tests Review of attachment 8724379 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/plugins/test/mochitest/browser_bug1163570.js @@ +56,3 @@ > let doc = content.document; > let plugin = doc.getElementById("testplugin"); > + Assert.equal(!!plugin, true, "plugin is loaded"); Assert.ok is probably better for this. @@ +82,3 @@ > let doc = content.document; > let plugin = doc.getElementById("testplugin"); > + Assert.equal(XPCNativeWrapper.unwrap(plugin).nativeWidgetIsVisible(), true, Assert.ok @@ +93,3 @@ > let doc = content.document; > let plugin = doc.getElementById("testplugin"); > + Assert.equal(XPCNativeWrapper.unwrap(plugin).nativeWidgetIsVisible(), false, Assert.ok(!

(In reply to Mike de Boer [:mikedeboer] from comment #11) > Jim, I was going over this whilst working on this bug, but it seems like > this is a bug: > > https://hg.mozilla.org/mozilla-central/diff/103700d6b355/browser/base/ > content/test/plugins/browser_blocking.js#l1.354 > > Which you introduced in bug 1129040: the return value of ContentTask.spawn > is _always_ true-ish, because it returns a Promise. I think you forgot to > put a `yield` there. > However, the try run in comment 10 seems to suggest that you expect `result` > to be `true`, but it's not. > Is it safe for me to turn the assertion around to expect `false`? It should be true if you add the yield. Looks I copy pasted that to a couple tests. I'll file a bug.

(In reply to Mike Conley (:mconley) - Needinfo me! from comment #20) > Huh... is this testing the right thing? Apparently, "Plugin should not be > activated", and yet here it is, being tested as activated, and probably > passing on our CI. > > Can you please file a follow-up to investigate this? Either the string is > wrong, or we're busted. This is a typo introduced in https://hg.mozilla.org/integration/fx-team/diff/103700d6b355/browser/base/content/test/plugins/browser_bug743421.js#l126. I've fixed it in the patch.

Carrying over r=mconley.

Carrying over r=mconley.

Carrying over r=mconley.

Comment on attachment 8725693 [details] [diff] [review] Patch 2.3: convert browser tests Review of attachment 8725693 [details] [diff] [review]: ----------------------------------------------------------------- Assuming a green try run, let's do it! Thanks mikedeboer! ::: browser/base/content/test/general/browser_bug561636.js @@ +72,5 @@ > } > > function* checkChildFocus(browser, message) > { > + message.testId = testId; No longer necessary to set testId on the message now.

https://hg.mozilla.org/integration/fx-team/rev/febf8025926dddc04ce8b74aff599b7ad37dda1a Bug 1241930: Part 1 - make all Assert.jsm assert methods available for use in ContentTasks. r=mconley https://hg.mozilla.org/integration/fx-team/rev/4247cb5030a6de2fd4cc98b42bc93b312bc2b6d5 Bug 1241930: Part 2 - convert all assertion methods used inside content tasks to the Assert.* family in browser tests. r=mconley https://hg.mozilla.org/integration/fx-team/rev/bf783d23f03ee7bdff1d06ee7f19d12d97f311a7 Bug 1241930: Part 3 - convert all assertion methods used inside content tasks to the Assert.* family in devtools tests. r=mconley https://hg.mozilla.org/integration/fx-team/rev/683d2e019f743f10594151ecd4bb5ec830aca7da Bug 1241930: Part 4 - convert all assertion methods used inside content tasks to the Assert.* family in docshell tests. r=mconley https://hg.mozilla.org/integration/fx-team/rev/92fd92a5c939b746e59e093211c542a63680edf3 Bug 1241930: Part 5 - convert all assertion methods used inside content tasks to the Assert.* family in dom tests. r=mconley https://hg.mozilla.org/integration/fx-team/rev/7ea58667c988dd692454325361aea810a3792f00 Bug 1241930: Part 6 - convert all assertion methods used inside content tasks to the Assert.* family in toolkit tests. r=mconley

https://hg.mozilla.org/mozilla-central/rev/febf8025926d https://hg.mozilla.org/mozilla-central/rev/4247cb5030a6 https://hg.mozilla.org/mozilla-central/rev/bf783d23f03e https://hg.mozilla.org/mozilla-central/rev/683d2e019f74 https://hg.mozilla.org/mozilla-central/rev/92fd92a5c939 https://hg.mozilla.org/mozilla-central/rev/7ea58667c988

Documented at https://wiki.mozilla.org/Electrolysis/e10s_test_tips#ContentTask