reftests currently run as a full suite of tests- this is good in many ways, but difficult for us to realistically seletively run directories in our CI system. Also it appears in bug 1300355 that a longer running test job for reftest is causing blank screenshots. I imagine this as an option that we can pass to the python command line. What makes this different from mochitest, etc. is that the manifest is parsed by javascript in-browser, so we would need to figure out a way to control the browser via preferences and some state between restarts, or find a way to parse the manifest in python and determine what we are running for each browser cycle. In terms of CI, this would then be turned on my adding the commandline option to the path for each respective config, it will create longer runtimes, but we can add a second chunk to reduce the overhead.

Hi Joel, The patch retrieved top manifest to sub items and then chunked them. For example, user can run "./mach reftest --total-chunk=10 --this-chunk=5" and only 1/10 manifests would be scheduled. If that is ok I can continue with it.

Comment on attachment 8859646 [details] [diff] [review] wip_patch Review of attachment 8859646 [details] [diff] [review]: ----------------------------------------------------------------- this is a reasonable start- I think handle more of the cases I outlined will help- I would like to see that first before diving into logistics of running the test. We will need to pass a parameter to reftest so it will ignore include statements. ::: layout/tools/reftest/runreftest.py @@ +190,5 @@ > + manifest_dir = os.path.dirname(manifest) > + import re > + with open(manifest, 'r') as manifest_fh: > + for line in manifest_fh.readlines(): > + found = re.search('include (.*list)', line) there are skip-if in includes: https://dxr.mozilla.org/mozilla-central/search?q=path%3A*.list+skip+include&redirect=true we should handle those in some way. In addition there is at least 1 comment that will cause problems: https://dxr.mozilla.org/mozilla-central/source/layout/reftests/reftest-sanity/prefix/urlprefixtests-include.list#4 and an odd named manifest that has include in the filename: https://dxr.mozilla.org/mozilla-central/source/layout/reftests/reftest-sanity/urlprefixtests-stylo.list#23 some cases of relative paths: https://dxr.mozilla.org/mozilla-central/source/layout/reftests/reftest-stylo.list#13 Also we can have: reftest-master.list include reftest-child.list reftest-child.list include reftest-childA.list so in your code you need to have a list of manifests and open each manifest to file all the child manifests.

(In reply to Joel Maher ( :jmaher) from comment #2) > Comment on attachment 8859646 [details] [diff] [review] > wip_patch > > Review of attachment 8859646 [details] [diff] [review]: > ----------------------------------------------------------------- > > this is a reasonable start- I think handle more of the cases I outlined will > help- I would like to see that first before diving into logistics of running > the test. We will need to pass a parameter to reftest so it will ignore > include statements. > Thanks. I'll try to handle those cases. > and an odd named manifest that has include in the filename: > https://dxr.mozilla.org/mozilla-central/source/layout/reftests/reftest- > sanity/urlprefixtests-stylo.list#23 > > some cases of relative paths: > https://dxr.mozilla.org/mozilla-central/source/layout/reftests/reftest-stylo. > list#13 > > > Also we can have: > reftest-master.list > include reftest-child.list > > reftest-child.list > include reftest-childA.list > > so in your code you need to have a list of manifests and open each manifest > to file all the child manifests. Not sure if we should recursively open child manifests since we might still pass parents, so I only retrieve top level manifest. Although reftest.jsm will make sure they're only enqueued once, top manifest which was chunked should not pass to test list directly.

a manifest can contain include and regular tests. We will need to pass to reftest a flag/preference so that it ignores includes. Of course this means we need some logic to ensure if there are no tests in a specific .list file (only includes) that we do not run that manifest.

(In reply to Joel Maher ( :jmaher) from comment #4) > a manifest can contain include and regular tests. We will need to pass to > reftest a flag/preference so that it ignores includes. Of course this means > we need some logic to ensure if there are no tests in a specific .list file > (only includes) that we do not run that manifest. Which means we need to implement recursive search mechanism in python. Though it might not be difficult we still need to maintain same logic in two languages. I'll keep trying another wip covering comment 2 & 4.

[update] skip-if has code which eval in sandbox. We can use marionette/execute_script, however marionette can only be run when firefox started. Therefore, complete manifest list can only be prepared after https://dxr.mozilla.org/mozilla-central/source/layout/tools/reftest/runreftest.py#638 But reftest profile and preferences including manifest lists are passed for launching browser. One thing can be considered is to launch build earlier for fetching skip-if conditions, and then shut down. I'll continue by launching build twice. Other issues are resolved.

Another approach to let js module help to parse manifest. Adding 2 options, - dryRun, ability to start reftest without actual test run. - ignoreInclude, enabled and include statements are ignored. We can still implement conditions parser but need to be careful about backward compatible. Other possibilities are like - Refactor reftest.jsm and separate its manifest parsing mechanism into module. - Get replacement of current reftest conditions. We should have them already in mochitest or others.

Comment on attachment 8866232 [details] [diff] [review] resolve_manifest_by_reftest.jsm Review of attachment 8866232 [details] [diff] [review]: ----------------------------------------------------------------- this seems like a more long term patch, thanks for changing it to this new technique. For things like Android there will be a high cost in running the browser to parse the manifests. One option we could do is generate a list of manifests during the build as an artifact so we don't need to parse it. This approach seems very reasonable for desktop tests though! ::: layout/tools/reftest/reftest.jsm @@ +383,5 @@ > + // gTotalChunks = prefs.getIntPref("reftest.totalChunks"); > + // gThisChunk = prefs.getIntPref("reftest.thisChunk"); > + gTotalChunks = 0; > + gThisChunk = 0; > + gIgnoreInclude = prefs.getBoolPref("reftest.ignoreInclude", false); why is this in a try when dryRun is not? also do we use chunks anymore? I would like to preserve that functionality, so this will have to be cleaned up. @@ +882,4 @@ > aFilter = [aFilter[0], aFilter[1], true]; > } > gManifestsLoaded[aURL.spec] = aFilter[1]; > + logger.info("Manifest[" + aURL.spec + "] was loaded with inherited_status[" + inherited_status + "]"); should this be conditional on dry run? ::: layout/tools/reftest/reftestcommandline.py @@ +228,5 @@ > + self.add_argument("--ignore-include", > + action="store_true", > + dest="ignoreInclude", > + default=False, > + help="Ignore include syntax expanded in reftest.jsm") do we need this commandline for the python bits? I would think we would have --run-by-manifest instead which would force reftest.jsm to use ignore-include ::: layout/tools/reftest/runreftest.py @@ +71,5 @@ > + return [] > + if data.has_key('message'): > + m = re.findall('\[([^\[]+)\]', data['message']) > + if m: > + self.manifests.append((m)) this doesn't look very reliable as we could get other data in here. What about dealing with full paths vs relative paths? @@ +462,5 @@ > + manifests = [] > + fetched_manifests = [] > + def fetch_manifests(message): > + if message.has_key("message") and 'Manifest' in message['message']: > + m = re.findall('\[([^\[]+)\]', message['message']) sam as above, this seems like magic and prime for getting other data. I would like to validate that we are getting real files we can find on the filesystem before appending it to fetched_manifests. @@ +478,3 @@ > manifests = self.resolver.resolveManifests(options, tests) > + > + profile = self.createReftestProfile(options, manifests, dryRun=True) we have a dryrun parameter in createReftestProfile()? @@ +490,5 @@ > + > + while len(self.log.handlers) > 1: > + self.log.handlers.pop() > + > + manifests = self.resolver.resolveManifests(options, fetched_manifests) overall this chunk of code should be in a function so we can |manifests = getReftestManifests(...)| @@ +502,5 @@ > + for i in xrange(options.totalChunks)][options.thisChunk] > + manifests = {i[0]: i[1] for i in this_manifests} > + > + import pdb > + pdb.set_trace() we won't want to land pdb

(In reply to Joel Maher ( :jmaher) from comment #8) > Comment on attachment 8866232 [details] [diff] [review] > resolve_manifest_by_reftest.jsm > > Review of attachment 8866232 [details] [diff] [review]: > ----------------------------------------------------------------- > > this seems like a more long term patch, thanks for changing it to this new > technique. For things like Android there will be a high cost in running the > browser to parse the manifests. One option we could do is generate a list > of manifests during the build as an artifact so we don't need to parse it. > This approach seems very reasonable for desktop tests though! > I think to collect manifest on build time really make sense. Thanks for point it out. Also thanks for reviewing the nits. > ::: layout/tools/reftest/reftest.jsm > @@ +383,5 @@ > > + // gTotalChunks = prefs.getIntPref("reftest.totalChunks"); > > + // gThisChunk = prefs.getIntPref("reftest.thisChunk"); > > + gTotalChunks = 0; > > + gThisChunk = 0; > > + gIgnoreInclude = prefs.getBoolPref("reftest.ignoreInclude", false); > > why is this in a try when dryRun is not? also do we use chunks anymore? I > would like to preserve that functionality, so this will have to be cleaned > up. > It's a experiment for other chunking estimation. I'll preserve later however worth of trial. > @@ +882,4 @@ > > aFilter = [aFilter[0], aFilter[1], true]; > > } > > gManifestsLoaded[aURL.spec] = aFilter[1]; > > + logger.info("Manifest[" + aURL.spec + "] was loaded with inherited_status[" + inherited_status + "]"); > > should this be conditional on dry run? > Yes. Will add check for it. > ::: layout/tools/reftest/reftestcommandline.py > @@ +228,5 @@ > > + self.add_argument("--ignore-include", > > + action="store_true", > > + dest="ignoreInclude", > > + default=False, > > + help="Ignore include syntax expanded in reftest.jsm") > > do we need this commandline for the python bits? I would think we would > have --run-by-manifest instead which would force reftest.jsm to use > ignore-include > Thanks I think we should fix option name. > ::: layout/tools/reftest/runreftest.py > @@ +71,5 @@ > > + return [] > > + if data.has_key('message'): > > + m = re.findall('\[([^\[]+)\]', data['message']) > > + if m: > > + self.manifests.append((m)) > > this doesn't look very reliable as we could get other data in here. What > about dealing with full paths vs relative paths? > > @@ +462,5 @@ > > + manifests = [] > > + fetched_manifests = [] > > + def fetch_manifests(message): > > + if message.has_key("message") and 'Manifest' in message['message']: > > + m = re.findall('\[([^\[]+)\]', message['message']) > > sam as above, this seems like magic and prime for getting other data. I > would like to validate that we are getting real files we can find on the > filesystem before appending it to fetched_manifests. > Not sure if I got the idea. We can give stronger pattern but if it is for file path, js module actually return full path like 'file://gecko_dir/layout/reftest/reftest.list' being provided by https://dxr.mozilla.org/mozilla-central/source/layout/tools/reftest/reftest.jsm#1091 > @@ +478,3 @@ > > manifests = self.resolver.resolveManifests(options, tests) > > + > > + profile = self.createReftestProfile(options, manifests, dryRun=True) > > we have a dryrun parameter in createReftestProfile()? > I can merge it into options but will create another commandline argument. Not sure if adding dryRun for command line is a good idea. > @@ +490,5 @@ > > + > > + while len(self.log.handlers) > 1: > > + self.log.handlers.pop() > > + > > + manifests = self.resolver.resolveManifests(options, fetched_manifests) > > overall this chunk of code should be in a function so we can |manifests = > getReftestManifests(...)| > > @@ +502,5 @@ > > + for i in xrange(options.totalChunks)][options.thisChunk] > > + manifests = {i[0]: i[1] for i in this_manifests} > > + > > + import pdb > > + pdb.set_trace() > > we won't want to land pdb Thanks I will remove this.

if we went the build system artifact route that would simplify a lot of things in this code, although that requires work in a different module and mozharness changes to download the manifest list. My concern with the regex is that you could end up with: * a file that is not found in the filesystem (invalid name, wrong path, relative path, etc.) * random text that is not a filename I think just validating each file that you get to verify the file exists would solve all my worries.

One thing why we re-use js module's parser is because its skip-if conditions. reftest created sandbox through https://dxr.mozilla.org/mozilla-central/source/layout/tools/reftest/reftest.jsm#620 so that it can accept condition like: https://dxr.mozilla.org/mozilla-central/source/layout/reftests/writing-mode/reftest.list#181 which was to eval piece of js code. To use existing build artifact is good however we might need to re-build parser aligned with conditionSandbox. -- If it is ok I think re-write by manifest_parser is doable, which means it might take out js's parsing function.

the goal here is to find manifests- so there is only solving the conditional manifests: https://dxr.mozilla.org/mozilla-central/search?q=path%3A*.list+include+skip-if&redirect=true In that list I see: * android * webrtc * windows 7 * release_or_beta (i.e. version) * browserIsRemote (e10s) * jsreftest conditions for features this.hasOwnProperty() or .prototpye(..) there could also be other instances in the future. Maybe just validate the files exist for all the data you find

[update] - Add path exists check. - Use option "run-by-manifests" to replace "ignoreInclude" - Minor fixes. For fennec I didn't actually come out ways, perhaps need a different approach.

Comment on attachment 8870259 [details] [diff] [review] resolve_manifest Review of attachment 8870259 [details] [diff] [review]: ----------------------------------------------------------------- a little nit in your code, also please ignore this for fennec- restarting the browser takes a long time and for mochitest we do not do --run-by-dir. ::: layout/tools/reftest/reftest.jsm @@ +582,5 @@ > gURICanvases = {}; > + if (!gDryRun) { > + StartCurrentTest(); > + } > + else { nit, we typically next statements, so make this: } else {

Bug 1344991 added a gStartAfter variable to reftest.jsm that allows the JS side to pick back up after a certain test. We could probably re-use that mechanism here.

Comment on attachment 8870259 [details] [diff] [review] resolve_manifest Hi ahal, according to discussion, please help to see what we can add in the patch, thanks.

Comment on attachment 8870259 [details] [diff] [review] resolve_manifest Review of attachment 8870259 [details] [diff] [review]: ----------------------------------------------------------------- So as far as I can tell, this is only going to cause the reftest harness to restart the browser once per chunk, but that's not really what we're going for in this bug. We need to restart the browser for every manifest, while still preserving the chunks (the same set of tests need to stay in the same jobs). After looking at this problem a bit, I'm becoming more convinced that we either need to do all the manifest parsing + chunking in JS or move them both out of JS and into python. I'm starting to think we might need to move it into python :/. Either way, this is a pretty hard problem and will be a decent amount of work. ::: layout/tools/reftest/reftest.jsm @@ +880,5 @@ > aFilter = [aFilter[0], aFilter[1], true]; > } > gManifestsLoaded[aURL.spec] = aFilter[1]; > + if (gDryRun) { > + logger.info("Manifest[" + aURL.spec + "] was loaded with inherited_status[" + inherited_status + "]"); Instead of using logger.info, we should re-use the logger.suiteStart message. The suiteStart action can take a dictionary like this: { 'manifest1': ['test1', 'test2', test3'], 'manifest2': ['test4', 'test5', 'test6'] } This will need to get built up in ReadManifest somewhere. This patch will also cause multiple suiteStart messages to get logged which will need to get fixed. @@ +1098,4 @@ > throw "Error in manifest file " + aURL.spec + " line " + lineNo + ": incorrect number of arguments to include"; > if (runHttp) > throw "Error in manifest file " + aURL.spec + " line " + lineNo + ": use of include with http"; > + if (!gIgnoreInclude) { I think we still want to runByManifests for sub manifests too? Maybe it's ok to punt on that for now. ::: layout/tools/reftest/runreftest.py @@ +484,5 @@ > + if options.thisChunk > 0 and options.totalChunks >= options.thisChunk: > + items = manifests.items() > + this_manifests = [items[i::options.totalChunks] > + for i in xrange(options.totalChunks)][options.thisChunk] > + manifests = {i[0]: i[1] for i in this_manifests} This isn't going to work, the chunking needs to operate on individual tests, not manifests. But I'm not sure that will be possible in this patch.

Thanks, I think it became more clear after your comment. (In reply to Andrew Halberstadt [:ahal] from comment #18) > Comment on attachment 8870259 [details] [diff] [review] > resolve_manifest > > Review of attachment 8870259 [details] [diff] [review]: > ----------------------------------------------------------------- > > So as far as I can tell, this is only going to cause the reftest harness to > restart the browser once per chunk, but that's not really what we're going > for in this bug. We need to restart the browser for every manifest, while > still preserving the chunks (the same set of tests need to stay in the same > jobs). > > After looking at this problem a bit, I'm becoming more convinced that we > either need to do all the manifest parsing + chunking in JS or move them > both out of JS and into python. I'm starting to think we might need to move > it into python :/. Either way, this is a pretty hard problem and will be a > decent amount of work. Out of JS is an option and but I'm not sure how we can backward-compatible with skip-if condition. Since they're all defined in reftest.jsm and are js syntax, we probably need to replace all of these, or write specific resolver for them. Just re-visiting bug 1300355 and it doesn't come to conclusion run-by-dir can resolves perfectly. from Ryan's comment https://bugzilla.mozilla.org/show_bug.cgi?id=1300355#c93 current chunking mechanism did help to reduce intermittent but add overhead which I think chunk by dir might have as well.

(In reply to Paul Yang [:pyang] from comment #19) > Out of JS is an option and but I'm not sure how we can backward-compatible > with skip-if condition. Since they're all defined in reftest.jsm and are js > syntax, we probably need to replace all of these, or write specific resolver > for them. Good point! We would need to try to parse it with a marionette execute_script call before running the main harness. But I think you mentioned you looked into this too, and I agree this won't be easy due to all the global state in the reftest harness. It would be possible, but would be a pretty big refactor. I think I might spend a bit of time to see how hard it would be to implement this purely in JS. > Just re-visiting bug 1300355 and it doesn't come to conclusion run-by-dir > can resolves perfectly. > from Ryan's comment https://bugzilla.mozilla.org/show_bug.cgi?id=1300355#c93 > current chunking mechanism did help to reduce intermittent but add overhead > which I think chunk by dir might have as well. Yes this is correct, run-by-dir will add some overhead to the harness. But it will be better value, since unlike chunking it will: A) Restart the browser more often B) Not incur the setup/teardown costs of running a whole new job each time (i.e setting up taskcluster workers)

That last push is my attempt at solving this purely in reftest.jsm. It mostly works, save for a big gotcha: reftest apparently doesn't have unique ids. There are at least two instance that I've found where the same test id is defined in two separate manifests. This is a problem because when we slice gURLs using the gStartAfter pref, it'll stop at the first test id instead of the second one, and then infinitely loop running the same slice of tests over and over. To fix this we can either: A) Prevent duplicate test ids (probably good to do anyway, but could mean renaming test files) B) Don't re-use gStartAfter, and instead use something like gNextManifest (might be easier, but increases complexity) I'll see how far I get with A) in a bit.

Might not have cycles for this in this quarter.

I got really close! The problem now is that there are a few tests that have different ids between browser restarts, for example they get a random port number assigned to them. This is bad for things like ActiveData anyway, so maybe I just need to hunt these down and try to fix them. But if we can't depend on test ids being the same across restarts, then --run-by-manifest is going to be very hard to implement properly.

I took another look at this last week and I think my attached approach is just going to be too flaky to rely on. I'm now fairly convinced that the best way to tackle this is to parse the manifests outside of reftest.jsm and store all the test/manifest information in python. Maybe we can use xpcshell or marionette. This is bug 1392391. Because moving the manifests out and implementing runByManifest is going to be a pretty large refactor, I'd also like to stand up some reftest selftests first (for the python parts, not reftest-sanity). Even if there isn't comprehensive coverage, just having a place to add tests for errors along the way will be very helpful. This is bug 1392390.

With bug 1392391 about ready to land, we're finally at a point where we can implement this!

One thing I'd like to do as part of this bug, is remove the start-after-crash functionality introduced by bug 1344991. I want to do this because: 1) It's mostly redundant with run-by-manifest. With run-by-manifest, if there's a crash we'll only skip running the rest of the tests later on in the manifest (not the entire suite). 2) It's potentially buggy. Reftest test ids aren't unique, so if we get unlucky and the wrong test perma-crashes, we could hit an infinite loop of tests (until taskcluster kills the task). 3) It adds a lot of complexity. There's a bunch of logic both in python and javascript to deal with this. Introducing yet another stop and restart based construct on top of this feature will make things even more convoluted. Even given 1), start-after-crash does have some value. It's not the feature I'm opposed to so much as the implementation. Now that we can pass in a list of tests to the JS harness (implemented by bug 1392391), it shouldn't be too hard to re-implement start-after-crash to use this feature in a way that is less buggy and flows well with run-by-manifest. I'm tempted to leave this re-implementation to a follow-up, but if there are strong objections I don't mind doing it as part of this bug.

Here's my preliminary try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=dd457903090c7eaac301f5d09c8202c80290a936 We're expecting some test failures from tests which depend on previous state set by other tests (and which are no longer run together). We'll have to triage and decide whether to disable or block on a fix like we normally do when standing up a new configuration.

I figure while we're waiting for those failures to be triaged, might as well get the review process underway. I'll purposely leave the annotation change unflagged until we have a better idea which failures we can disable, and which we should block on.

Comment on attachment 8952779 [details] Bug 1353461 - [manifestparser] Implement a chunk_by_manifest algorithm, https://reviewboard.mozilla.org/r/222002/#review227896 this seems very logical- do we need to publish a manifestparser package? I recall in some experiments with run_by_component I couldn't modify manifestparser to add a new filter without a need to update in a few places.

Comment on attachment 8952780 [details] Bug 1353461 - [reftest] remove the start-after-crash feature, https://reviewboard.mozilla.org/r/222004/#review227904 excellent

Comment on attachment 8952781 [details] Bug 1353461 - [reftest] Implement run-by-manifest for reftest, https://reviewboard.mozilla.org/r/222006/#review227908 three r+ in a row, this is looking good

In a couple blockers devs had said go ahead and fails/random/fuzzy-if the tests out. A couple others are being actively investigated. We should at least block on bug 1399746 as that already has a patch up for review and it was the one that required us to skip an entire manifest.

There are still a couple of test fixes that are only on inbound, so we'll have to wait for them to make their way to autoland before landing. We only ended up needing to disable two tests (one of which we have go ahead from devs, and the other is already disabled in a ton of places due to flakiness). There are also a handful of new intermittents that show up with this, but I think we should let those fall into the normal sheriffing workflow. Overall, this change should fix many more intermittents than it causes. I'll be PTO for two weeks starting next week. So hopefully I'll remember to land this tonight or tomorrow and it doesn't get backed out. Otherwise if there are test/intermittent issues, please ask :jmaher and/or :RyanVM.

Comment on attachment 8952782 [details] Bug 1353461 - Chunk OSX debug reftests to 3, https://reviewboard.mozilla.org/r/222008/#review229004

Comment on attachment 8953639 [details] Bug 1353461 - Increase fuzzy-if for layout/reftests/svg/foreignObject-form-theme.svg with webrender, https://reviewboard.mozilla.org/r/222868/#review229006

Comment on attachment 8953640 [details] Bug 1353461 - Mark toolkit/content/tests/reftests/bug-442419-progressmeter-max.xul fails on Windows, https://reviewboard.mozilla.org/r/222870/#review229008

Pushed by jmaher@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/097761a4d06c [manifestparser] Implement a chunk_by_manifest algorithm, r=jmaher https://hg.mozilla.org/integration/autoland/rev/c61a2a0ac22f [reftest] remove the start-after-crash feature, r=jmaher https://hg.mozilla.org/integration/autoland/rev/3585c6359fa2 [reftest] Implement run-by-manifest for reftest, r=jmaher https://hg.mozilla.org/integration/autoland/rev/6b584d340491 Chunk OSX debug reftests to 3, r=jmaher https://hg.mozilla.org/integration/autoland/rev/cc3f64bb1878 Increase fuzzy-if for layout/reftests/svg/foreignObject-form-theme.svg with webrender, r=jmaher https://hg.mozilla.org/integration/autoland/rev/673a847ac360 Mark toolkit/content/tests/reftests/bug-442419-progressmeter-max.xul fails on Windows, r=jmaher

https://hg.mozilla.org/mozilla-central/rev/097761a4d06c https://hg.mozilla.org/mozilla-central/rev/c61a2a0ac22f https://hg.mozilla.org/mozilla-central/rev/3585c6359fa2 https://hg.mozilla.org/mozilla-central/rev/6b584d340491 https://hg.mozilla.org/mozilla-central/rev/cc3f64bb1878 https://hg.mozilla.org/mozilla-central/rev/673a847ac360