Closed Bug 1405349 Opened 5 years ago Closed 5 years ago

Refactor reftest manifest parsing code into a separate JSM

Categories

(Testing :: Reftest, enhancement)

Product:
Testing
Component:
Reftest
Version:
Version 3
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

(firefox58 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla58
Tracking Flags:
Tracking Status
firefox58 --- fixed

People

(Reporter: ahal, Assigned: ahal)

References

Details

Attachments

(1 file)

Bug 1405349 - [reftest] Refactor manifest parsing from reftest.jsm to standalone module,
59 bytes, text/x-review-board-request
jmaher
: review+
Details 
The JavaScript portion of the reftest harness lives mostly in a single large file (reftest.jsm). It handles everything in the chrome side, from parsing manifests, to loading tests, to capturing logs and results + much more. It also makes very heavy use of globals for keeping track of state.

This pattern has made it very tricky to both understand reftest, and extend it without introducing subtle bugs. A worthy goal would be to A) separate logical pieces into several independent JSM's, and B) try to avoid relying on global state. A good first piece to factor out is all the manifest parsing logic. As a first iteration, I think it can still share global state with "reftest.jsm", and we can look to improve the situation in follow-up bugs.

This isn't just a refactor for refactors sake. The end goal is to support "run-by-manifest" mode in reftest (bug 1353461). But this particular bug seems worth doing either way.
Comment on attachment 8915585 [details]
Bug 1405349 - [reftest] Refactor manifest parsing from reftest.jsm to standalone module,

Hey dbaron, no need to review this in depth (unless you want to), but wanted to make sure you were ok with this general approach. The commit message explains the motivation for this change.

Basically it pulls all manifest parsing related code into a manifest.jsm module. Since manifest parsing shares a decent amount of global state, it also pulls all global variables (and constants) into a globals.jsm module. I'm not sure if this is the best way to share state between the two modules or not though.

Thanks in advance.
Attachment #8915585 - Flags: feedback?(dbaron)
Comment on attachment 8915585 [details]
Bug 1405349 - [reftest] Refactor manifest parsing from reftest.jsm to standalone module,

Yeah, I'm ok with the general approach as long as it's still possible to run the reftest harness without using the python parts; that's sometimes useful when working with debugging tools, or when doing interesting things (e.g., using it to screenshot CSSWG tests).
Attachment #8915585 - Flags: feedback?(dbaron) → feedback+
(In reply to David Baron :dbaron: ⌚️UTC-7 from comment #4)
> Comment on attachment 8915585 [details]
> Bug 1405349 - [reftest] Refactor manifest parsing from reftest.jsm to
> standalone module,
> 
> Yeah, I'm ok with the general approach as long as it's still possible to run
> the reftest harness without using the python parts; that's sometimes useful
> when working with debugging tools, or when doing interesting things (e.g.,
> using it to screenshot CSSWG tests).

Thanks, yeah this shouldn't change anything around that. The follow-up bug 1392391 might, but I'll keep this workflow in mind and try to make sure both cases still work.
Comment on attachment 8915585 [details]
Bug 1405349 - [reftest] Refactor manifest parsing from reftest.jsm to standalone module,

https://reviewboard.mozilla.org/r/186780/#review194106

a few nits below- overall this looks as though large chunks of variables and code are moved without any structural changes.  A few nits which could be addressed in a followup if there are actions to take.  Overall I do not like the use of 'g' as a variable, but it is easy to read in the modified code and do not think we should change it.

::: layout/tools/reftest/globals.jsm:19
(Diff revision 2)
> +  NS_GFXINFO_CONTRACTID: "@mozilla.org/gfx/info;1",
> +  IO_SERVICE_CONTRACTID: "@mozilla.org/network/io-service;1",
> +  DEBUG_CONTRACTID: "@mozilla.org/xpcom/debug;1",
> +  NS_LOCALFILEINPUTSTREAM_CONTRACTID: "@mozilla.org/network/file-input-stream;1",
> +  NS_SCRIPTSECURITYMANAGER_CONTRACTID: "@mozilla.org/scriptsecuritymanager;1",
> +  NS_REFTESTHELPER_CONTRACTID: "@mozilla.org/reftest-helper;1",

NS_REFTESTHELPER_CONTRACTID and NS_LOCALFILEINPUTSTREAM_CONTRACTID and NS_LOCAL_FILE_CONTRACTID isn't used at all.

::: layout/tools/reftest/globals.jsm:21
(Diff revision 2)
> +  DEBUG_CONTRACTID: "@mozilla.org/xpcom/debug;1",
> +  NS_LOCALFILEINPUTSTREAM_CONTRACTID: "@mozilla.org/network/file-input-stream;1",
> +  NS_SCRIPTSECURITYMANAGER_CONTRACTID: "@mozilla.org/scriptsecuritymanager;1",
> +  NS_REFTESTHELPER_CONTRACTID: "@mozilla.org/reftest-helper;1",
> +  NS_NETWORK_PROTOCOL_CONTRACTID_PREFIX: "@mozilla.org/network/protocol;1?name=",
> +  NS_XREAPPINFO_CONTRACTID: "@mozilla.org/xre/app-info;1",

NS_NETWORK_PROTOCOL_CONTRACTID_PREFIX and NS_XREAPPINFO_CONTRACTID and NS_SCRIPTSECURITYMANAGER_CONTRACTID and IO_SERVICE_CONTRACTID are only used in manifest.jsm.

::: layout/tools/reftest/globals.jsm:58
(Diff revision 2)
> +
> +  // "<!--CLEAR-->"
> +  BLANK_URL_FOR_CLEARING: "data:text/html;charset=UTF-8,%3C%21%2D%2DCLEAR%2D%2D%3E",
> +
> +  RE_PROTOCOL: /^\w+:/,
> +  RE_PREF_ITEM: /^(|test-|ref-)pref\((.+?),(.*)\)$/,

this two RE_P* variables are only used in the new manifest.jsm?  should we move it there?  Also previously this was a const, should I assume that the lack of 'const' defaults to a 'const' value?

::: layout/tools/reftest/reftest.jsm:516
(Diff revision 2)
> -    gURIUseCounts = {};
> -    for (var i = 0; i < gURLs.length; ++i) {
> -        var url = gURLs[i];
> +    g.uriUseCounts = {};
> +    for (var i = 0; i < g.urls.length; ++i) {
> +        var url = g.urls[i];
>          if (url.expected != EXPECTED_DEATH &&
>              (url.type == TYPE_REFTEST_EQUAL ||
>               url.type == TYPE_REFTEST_NOTEQUAL)) {

these work ok in this patch, but any manifest parsing in multiple stages or different processes will present a challenge here.
Attachment #8915585 - Flags: review?(jmaher) → review+
Comment on attachment 8915585 [details]
Bug 1405349 - [reftest] Refactor manifest parsing from reftest.jsm to standalone module,

https://reviewboard.mozilla.org/r/186780/#review194106

I'm not crazy about using 'g' either. But in order for mutated variables to get updated across modules (i.e so if reftest.jsm modifies a variable, manifest.jsm will see it too), we need to use an object as that is passed by reference. Using a longer name would make the module less readable due to how often globals are used. There might be some kind of JS magic to avoid using a container object, but if there is it's beyond my knowledge.

> this two RE_P* variables are only used in the new manifest.jsm?  should we move it there?  Also previously this was a const, should I assume that the lack of 'const' defaults to a 'const' value?

I don't think it will be assigned with `const`. I'm not sure how to do that.
Pushed by ahalberstadt@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3cf260e1fb9c
[reftest] Refactor manifest parsing from reftest.jsm to standalone module, r=jmaher
So our current best idea is to move the Windows 7 debug non-e10s reftests to Windows 10. Things are a lot more stable over there. I'll file a new bug for this.
Depends on: 1408505
Here's the latest try run:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=be40b50fa3df876b63ce993c6b7f0eff1703eaeb

Moving the non-e10s reftests to Windows 10 seems to have helped (they were also retriggered a lot more in a previous try push). I think we're good to attempt a second landing.
Pushed by ahalberstadt@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1083fb72eed4
[reftest] Refactor manifest parsing from reftest.jsm to standalone module, r=jmaher
https://hg.mozilla.org/mozilla-central/rev/1083fb72eed4
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
status-firefox58: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in before you can comment on or make changes to this bug.