Closed Bug 1074507 Opened 10 years ago Closed 10 years ago

Make it possible to skip tests if not running with --e10s.

Categories

(Testing :: Mochitest, defect)

x86
All
defect
Not set
normal
Points:
2

Tracking

(e10s+)

RESOLVED FIXED
mozilla35
Iteration:
35.3
Tracking Status
e10s + ---

People

(Reporter: mconley, Assigned: markh)

References

(Blocks 2 open bugs)

Details

(Whiteboard: [qa-])

Attachments

(1 file, 1 obsolete file)

We currently have the ability to skip Mochitest tests using the following syntax in our ini files:

skip-if = e10s

I'm starting to write tests for various things that only work if we're running our tests with the --e10s flag. We should make it possible to use:

skip-if = !e10s

Unfortunately, the above does not currently work. If I set that in a .ini file, and run the test with --e10s, it reports:

4 INFO TEST-UNEXPECTED-FAIL | browser-test.js | No tests to run. Did you pass an invalid --test-path?
Not sure why that happens, but maybe try "run-if = e10s" instead?
run-if = e10s is equivalent to skip-if = !e10s (and we're trying to deprecate run-if to make it less confusing).
The same error message gets reported. :(
Blocks: 1070096
My memory is getting vague, but I believe this has to do with when the tests are initially filtered.  The filtering basically assumes everything needed to filter is known at build config time (eg, from in config.status).  "e10s" is a little special because that variable basically springs into life *after* the initial filtering, but *before* a second set of filtering done.

So, in the case we know works - skip-if = e10s:

* First set of filtering is done.  e10s is false (as we are looking for it as a build-time variable) - so test is not skipped and is copied to all-tests.json.
* Second set of filtering is done - but by this time we've parse the command-line, so find the real value for e10s - and the skips are honoured.

In the case that doesn't work - skip-if = !e10s
* First set of filtering is done.  !e10s is true, so the tests *are not* copied in this first filtering phase (so don't appear in all-tests.json)
* Second set of filtering is done - but it's too late here - we've already excluded the tests are care about.

I'm sure I opened a bug about a year ago on this, but can't find it now.
Sounds like we might need to special-case "e10s" in that first filtering step so that we leave it the hell alone at build time.
Or rather, at build time, have e10s always be "true", and then let the second phase of filtering do the actual selection.
Hey ahal, any advice on any of the above?
Flags: needinfo?(ahalberstadt)
Attached patch t.patch (obsolete) — Splinter Review
If my understanding is correct, the "problem" is at http://mxr.mozilla.org/mozilla-central/source/python/mozbuild/mozbuild/frontend/emitter.py#764 - if we could avoid this pre-filtering I think we'd be good.  However, it's not clear to me how many of the harnesses have this problem.

I quickly changed the boolean flag to pre-filter, and my super-quick testing shows it works fine - skip-if=e10s for a single test worked.  Mike, maybe you could play with this patch and see if you can spot anything that breaks?
Comment on attachment 8497291 [details] [diff] [review]
t.patch

I think as per the comment above we can actually remove that parameter entirely nowadays. That's a holdover from when we started defining Mochitests in manifests but the harness wasn't actually running from the manifests.
+1 to Mark and Ted's proposal. Build time filtering causes problems for harnesses and tools that operate outside the build system, so anything that gets us away from that model is a good thing imo.
Flags: needinfo?(ahalberstadt)
A note that somebody suggested a while back of writing tests such that we open a new e10s browser, regardless of whether or not we're testing with --e10s, and then do testing / manipulations in the new window.

While that workaround would work for tests running with --e10s and without, it's actually not great because we don't get the shims for things like load events, since browser.tabs.remote.autostart would be false.
Comment on attachment 8497291 [details] [diff] [review]
t.patch

This seems to do the job for me. ted, are you the right one to review this?
Attachment #8497291 - Flags: review?(ted)
This is the same basic patch, although that boolean has been removed entirely from the tuple and all comments relating to it removed.

Try at https://tbpl.mozilla.org/?tree=Try&rev=debd1a0f9f4c
Attachment #8497291 - Attachment is obsolete: true
Attachment #8497291 - Flags: review?(ted)
Attachment #8498548 - Flags: review?(ted)
Comment on attachment 8498548 [details] [diff] [review]
avoid pre-filtering

Review of attachment 8498548 [details] [diff] [review]:
-----------------------------------------------------------------

Do we not have any tests for this behavior?
Attachment #8498548 - Flags: review?(ted) → review+
Thanks.

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #14)
> 
> Do we not have any tests for this behavior?

Not that I could find.
Assignee: nobody → mhammond
Status: NEW → ASSIGNED
Points: --- → 2
Flags: firefox-backlog+
Keywords: checkin-needed
Whiteboard: [qa-]
Iteration: --- → 35.3
https://hg.mozilla.org/mozilla-central/rev/33b673fb5f99
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
You need to log in before you can comment on or make changes to this bug.