Closed Bug 1451513 Opened 6 years ago Closed 6 years ago

Convert mochitest extension from bootstrap to a webextension

Categories

(Testing :: Mochitest, enhancement, P1)

Product:
Testing
Component:
Mochitest
Version:
Version 3
Type:
enhancement

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: aswan, Assigned: aswan)

References

(Depends on 1 open bug)

Details

Attachments

(3 files)

Bug 1451513 Part 1: Allow registering chrome: content resources
59 bytes, text/x-review-board-request
kmag
: review+
Details
Bug 1451513 Part 2: Convert mochikit to a webextension
59 bytes, text/x-review-board-request
jmaher
: review+
kmag
: review+
Details
Bug 1451513 Part 3: Make loading the mochitest extension more robust
59 bytes, text/x-review-board-request
kmag
: review+
Details 
Depending on how much enthusiasm whoever ends up doing this has, some things could be modernized a bit or we could just do a quick-and-dirty conversion of the existing code to a WebExtension experiment.
I started looking into this and the first thing I noticed is that many tests directly load various scripts and other assets from chrome://mochikit/ urls.  But these assets can also be loaded from the mochi.test web server.  Given how widely used the chrome: urls are, I think it would be easiest to do a big scripted rewrite of these to regular http://mochi.test urls as a separate step before doing the rest of the extension conversion.
Before I start on that though, a sanity check would be good.  Joel, does this sound reasonable?  If so, I'll file a separate bug and aim to tackle it next week.
Flags: needinfo?(jmaher)
yes, if that works I would be fine with it; I would want to let it bake as possibly that could introduce other failures.
Flags: needinfo?(jmaher)
Agreed, separate step, please.
Depends on: 1473150
No longer depends on: 1473150
Assignee: nobody → aswan
Priority: -- → P1
Attachment #8990895 - Flags: review?(kmaglione+bmo)
Attachment #8990896 - Flags: review?(kmaglione+bmo)
Attachment #8990896 - Flags: review?(jmaher)
Comment on attachment 8990895 [details]
Bug 1451513 Part 1: Allow registering chrome: content resources

https://reviewboard.mozilla.org/r/255900/#review262744

::: toolkit/mozapps/extensions/test/xpcshell/test_registerchrome.js:44
(Diff revision 1)
>    equal(registry.convertChromeURL(overrideURL).spec, file2.spec);
> -  equal(registry.convertChromeURL(localeURL).spec, file1.spec + "/foo");
> +  let file = file1.spec + "/foo";
> +  equal(registry.convertChromeURL(contentURL).spec, file);
> +  equal(registry.convertChromeURL(localeURL).spec, file);
>  
> -  // After destroying the second entry, the first entry should not take
> +  // After destroying the second entry, the first entry should now take

Thanks.
Attachment #8990895 - Flags: review?(kmaglione+bmo) → review+
Comment on attachment 8990896 [details]
Bug 1451513 Part 2: Convert mochikit to a webextension

https://reviewboard.mozilla.org/r/255902/#review262746

::: testing/mochitest/api.js:140
(Diff revision 1)
> -}
> -
> -function install(data, reason) {}
> -function uninstall(data, reason) {}
>  
> +    if (this.chromeHandle) {

No need for the `if`. We'll never get an onShutdown if we haven't got an `onStartup`, and if we get an `onStartup`, we'll also have a `chromeHandle`.

::: testing/mochitest/manifest.json:11
(Diff revision 1)
> +  "applications": {
> +    "gecko": {
> +      "id": "mochikit@mozilla.org"
> +    }
> +  },
> +  "permissions": [],

Not necessary?

::: testing/mochitest/moz.build:18
(Diff revision 1)
> -FINAL_TARGET_PP_FILES += ['install.rdf']
> +FINAL_TARGET_FILES += [
> +    'api.js',
> +    'manifest.json',
> +    'schema.json',
> +]
> +
> +FINAL_TARGET_FILES.content += [
> +    'browser-harness.xul',
> +    'browser-test.js',
> +    'chrome-harness.js',
> +    'chunkifyTests.js',
> +    'harness.xul',
> +    'manifestLibrary.js',
> +    'mochitest-e10s-utils.js',
> +    'nested_setup.js',
> +    'redirect.html',
> +    'server.js',
> +    'shutdown-leaks-collector.js',
> +    'ShutdownLeaksCollector.jsm',
> +]
> +
> +FINAL_TARGET_FILES.content.dynamic += [
> +    'dynamic/getMyDirectory.sjs',
> +]
> +
> +FINAL_TARGET_FILES.content.static += [
> +    'static/harness.css',
> +]
>  
> -FINAL_TARGET_FILES += ['bootstrap.js']
> +FINAL_TARGET_FILES.content.tests.SimpleTest += [
> +    '../../docshell/test/chrome/docshell_helpers.js',
> +    '../modules/StructuredLog.jsm',
> +    '../specialpowers/content/MozillaLogger.js',
> +    '../specialpowers/content/specialpowers.js',
> +    '../specialpowers/content/specialpowersAPI.js',
> +    '../specialpowers/content/SpecialPowersObserverAPI.js',
> +    'tests/SimpleTest/AddTask.js',
> +    'tests/SimpleTest/AsyncUtilsContent.js',
> +    'tests/SimpleTest/ChromePowers.js',
> +    'tests/SimpleTest/EventUtils.js',
> +    'tests/SimpleTest/ExtensionTestUtils.js',
> +    'tests/SimpleTest/iframe-between-tests.html',
> +    'tests/SimpleTest/LogController.js',
> +    'tests/SimpleTest/MemoryStats.js',
> +    'tests/SimpleTest/MockObjects.js',
> +    'tests/SimpleTest/NativeKeyCodes.js',
> +    'tests/SimpleTest/paint_listener.js',
> +    'tests/SimpleTest/setup.js',
> +    'tests/SimpleTest/SimpleTest.js',
> +    'tests/SimpleTest/test.css',
> +    'tests/SimpleTest/TestRunner.js',
> +    'tests/SimpleTest/WindowSnapshot.js',
> +]
> +
> +FINAL_TARGET_FILES.content.tests.BrowserTestUtils += [
> +    'BrowserTestUtils/content/content-about-page-utils.js',
> +    'BrowserTestUtils/content/content-task.js',
> +    'BrowserTestUtils/content/content-utils.js',
> +]

Why can't we use jar.mn for this anymore?

::: testing/mochitest/redirect.html:27
(Diff revision 1)
>  
>      function onLoad() {
>        // Wait for MozAfterPaint, since the listener in browser-test.js is not
>        // added until then.
>        window.addEventListener("MozAfterPaint", function() {
> -        setTimeout(redirectToHarness, 0);
> +        // In case the listener is not ready, re-try periodically

Hm
Attachment #8990896 - Flags: review?(kmaglione+bmo) → review+
Comment on attachment 8990896 [details]
Bug 1451513 Part 2: Convert mochikit to a webextension

https://reviewboard.mozilla.org/r/255902/#review262802

nothing outside of what :kmag pointed out.
Attachment #8990896 - Flags: review?(jmaher) → review+
Comment on attachment 8990896 [details]
Bug 1451513 Part 2: Convert mochikit to a webextension

https://reviewboard.mozilla.org/r/255902/#review262746

> Why can't we use jar.mn for this anymore?

We could but that causes everything to be stuck in a chrome/ subdirectory inside the xpi.  Which I guess we could just handle but meh.

> Hm

The whole "retry in case the extension isn't ready yet" approach feels icky but I think this is a strict improvement.  The old code would fail if it took long enough to start up that it missed the 5 second timer.  This will still fail in the same way (the 6 minute timeout) if the extension completely fails to start, but be more robust if the extension is just really slow to start.
Comment on attachment 8990896 [details]
Bug 1451513 Part 2: Convert mochikit to a webextension

https://reviewboard.mozilla.org/r/255902/#review262746

> The whole "retry in case the extension isn't ready yet" approach feels icky but I think this is a strict improvement.  The old code would fail if it took long enough to start up that it missed the 5 second timer.  This will still fail in the same way (the 6 minute timeout) if the extension completely fails to start, but be more robust if the extension is just really slow to start.

Ah, however I botched this and removed the immediate attempt.  I'll fix this...
Comment on attachment 8990896 [details]
Bug 1451513 Part 2: Convert mochikit to a webextension

https://reviewboard.mozilla.org/r/255902/#review262746

> Ah, however I botched this and removed the immediate attempt.  I'll fix this...

OK. This might actually fix some of the intermittant startup timeouts we've been seeing, so I guess I can't complain.
Pushed by aswan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/afc5c4bd60da
Part 1: Allow registering chrome: content resources r=kmag
https://hg.mozilla.org/integration/autoland/rev/604167986098
Part 2: Convert mochikit to a webextension r=jmaher,kmag
https://hg.mozilla.org/mozilla-central/rev/afc5c4bd60da
https://hg.mozilla.org/mozilla-central/rev/604167986098
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox63: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Depends on: 1426822
This patch seem to have caused a massive spike of mochitest harness timeout failures (~400 per day) as tracked via bug 1414495.

Sebastian will backout this patch soon to verify it.
Backed out for likely causing frequent mochitest suite hangs (bug 1414495):

https://hg.mozilla.org/mozilla-central/rev/befd4afd29bf9c145701853d308a59b7fc37e6a4
Status: RESOLVED → REOPENED
status-firefox63: fixed → ---
Flags: needinfo?(aswan)
Resolution: FIXED → ---
Target Milestone: mozilla63 → ---
My try build [1] shows that the `mochitest-load` event is correctly dispatched by Marionette [2], but it is never received by the Mochitest extension. Maybe there is a race and the listener was not yet registered? 

Maybe installing a webextension is different from a bootstrapped extension and our `installAddon` method [3] in Marionette returns too early?

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=f84d59ed985dd6b3a3d5cd45ee66f7aeb71680c2&selectedJob=190035789
[2] https://treeherder.mozilla.org/logviewer.html#?job_id=190035789&repo=try&lineNumber=4240-4247
[3] https://searchfox.org/mozilla-central/source/testing/marionette/addon.js#23
Here the test results for the backout:

https://treeherder.mozilla.org/#/jobs?repo=mozilla-central&revision=befd4afd29bf9c145701853d308a59b7fc37e6a4&filter-searchStr=mochitest&selectedJob=190053843

It proves that my suspicion was correct and this bug was responsible for the massive mochitest timeout failures.
Unfortunately, the backout seems to have caused Android robocop to perma-fail:

https://treeherder.mozilla.org/#/jobs?repo=mozilla-central&filter-searchStr=robocop&fromchange=2ee0f455b43bb5e452793bb2c01ea2985e5ae6d8&tochange=12f42d129762967c3f912085787ad489ec97feaa

At a glance, it looks like each robocop test is timing out on shutdown, perhaps related to Robocop:Quit processing?? I'll have a look tomorrow.
Flags: needinfo?(gbrown)
...maybe bug 1451525 needs to be backed out also?
Bug 1451525 was dependent on the part 1 patch that was backed out here.
I'd appreciate if part 1 was re-landed instead of backing out 1451525.  Even better would have been giving me a chance to respond to the needinfo before backing out but its too late for that I guess...
Keywords: leave-open
(In reply to Andrew Swan [:aswan] from comment #21)
> Bug 1451525 was dependent on the part 1 patch that was backed out here.
> I'd appreciate if part 1 was re-landed instead of backing out 1451525.  Even

Sorry for that. It was an unfortunate timing, but also bug 1451525 was not listed as being blocked by this bug. So it was not clear that something else needs parts of this patch. Thanks Kris for relanding part 1.
Blocks: 1451525
Flags: needinfo?(gbrown)
Attachment #8996162 - Flags: review?(kmaglione+bmo)
Try run with the newly added patch and 3 iterations of mochitests on linux with no occurrences of bug 1414495:

https://hg.mozilla.org/try/rev/4b8459d0cae55f6da960b28ff67d60bf08440c7a
Comment on attachment 8996162 [details]
Bug 1451513 Part 3: Make loading the mochitest extension more robust

https://reviewboard.mozilla.org/r/260398/#review267582

::: testing/marionette/addon.js:31
(Diff revision 1)
> -
> -    let success = install => {
> +    let addon = await install.install();
> +    return addon;

Maybe just `return await ...` and `// eslint-disable-next-line` to silence the warning?
Attachment #8996162 - Flags: review?(kmaglione+bmo) → review+
Comment on attachment 8996162 [details]
Bug 1451513 Part 3: Make loading the mochitest extension more robust

https://reviewboard.mozilla.org/r/260398/#review267582

> Maybe just `return await ...` and `// eslint-disable-next-line` to silence the warning?

`installAddon()` is an async function, so we can directly return the return value from `install.install()`, or?
Comment on attachment 8996162 [details]
Bug 1451513 Part 3: Make loading the mochitest extension more robust

https://reviewboard.mozilla.org/r/260398/#review267582

> `installAddon()` is an async function, so we can directly return the return value from `install.install()`, or?

We could, but then the catch() block wouldn't catch the error, and the original error would be propagated to callers.

I suppose `return install.install().catch(...)` would probably be clearer, though.
Status: REOPENED → RESOLVED
Closed: 6 years ago → 6 years ago
Keywords: leave-open
Resolution: --- → FIXED
Flags: needinfo?(aswan)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: