when parsing test manifests, ensure we have an attribute to denote we are skipping test

RESOLVED FIXED in mozilla28

Status

RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: jmaher, Assigned: markh)

Tracking

unspecified
mozilla28
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

5 years ago
in the implementation of manifests for mochitest (bug 868158), we are reading a generating a json file of tests to run.  This is limited because we filter out tests which do not run based on platform or os features.  It would be nice to demark the test as 'skipped' and have the test runner interpret this.
(Assignee)

Comment 1

5 years ago
Created attachment 822064 [details] [diff] [review]
Write disabled tests to the manifest and have the manifest parser report and skip them

This is a relatively simple patch that writes the disabled tests to the manifest and has manifestLibrary.js detect and handle it.  The end result is that the test log starts with all the skipped tests and the reason - eg:

0:04.94 TEST-SKIPPED | browser/base/content/test/general/browser_CTP_context_menu.js | skip-if: e10s # Bug 899347 - no e10s click-to-play support
 0:04.94 TEST-SKIPPED | browser/base/content/test/general/browser_CTP_data_urls.js | skip-if: e10s # Bug 899347 - no e10s click-to-play support

(It would probably be better to report the skips inline as the test *would* have been run - possibly even reporting it as a "todo" - but that looks like a much more intrusive change).

It's quite likely I'm missing something significant that would prevent this from being viable, but it works in my very limited testing.  I suspect the xpcshell test runner would need something similar, but thought I'd see what people thought of it.

Asking for feedback from Joel as he opened the bug, but feel free to pass it on!
Attachment #822064 - Flags: feedback?(jmaher)
(Reporter)

Comment 3

5 years ago
Comment on attachment 822064 [details] [diff] [review]
Write disabled tests to the manifest and have the manifest parser report and skip them

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

::: testing/mochitest/manifestLibrary.js
@@ +21,5 @@
>      var path = obj['path'];
> +    if (obj.disabled) {
> +      dump("TEST-SKIPPED | " + path + " | " + obj.disabled + "\n");
> +      continue;
> +    }

this will dump this out at the top of the log file, not necessarily in the context.  It is ok, I would like to see if we could make this work in context before going this route.
Attachment #822064 - Flags: feedback?(jmaher) → feedback-
(Assignee)

Comment 4

5 years ago
(In reply to Joel Maher (:jmaher) from comment #3)
> this will dump this out at the top of the log file, not necessarily in the
> context.  It is ok, I would like to see if we could make this work in
> context before going this route.

I agree that would be nicer.  Assuming by "in context" you mean "reported in the log when they would have been run had they not been disabled", then a quick look implies the patch would look something like:

manifestParser.js:
* parseTestManifest would change from returning a list of test paths, to a list of objects with a (say) 'path' attribute and possibly a 'disabled' attribute.

* filterTests would need to be changed so that instead of returning just the list of tests that should be run, it would instead return the original list, but add a new 'disabled' attribute to the tests that have been filtered out.

TestRunner.js:
* Would change from using that TestRunner._urls (an array of strings) to, say, TestRunner._tests, which would be an array of objects, with a 'url' attribute and possibly a 'disabled' attribute.

* Instead of a simple TestRunner._currentTest++, the code would loop until it finds a test that doesn't have a 'disabled' attribute, reporting the disabled tests as it does so.  This would also need to be done at the very start (incase test[0] is disabled) and when the test runner is reset (when _currentTest is explicitly reset to 0)

* I *think* the 'chrome' test runner would need similar changes too (but I'm still getting my head around some of this testing infrastructure)

Does that sound correct?

That looks like it would be a relatively large and intrusive patch - which I can play with if you think it sounds likely to be accepted.  Another possible option that might be less intrusive would be to report the skipped tests at the end of the test run.

What do you think?
Flags: needinfo?(jmaher)
(Reporter)

Comment 5

5 years ago
yeah, that sounds correct and the more I think about what it will take the more I think it isn't worth the effort for minimal gain.  I suspect as we migrate towards a real manifest solution some of the code required will change and this might be an easier project to tackle in the near future.

Lets go with your original proposal, sorry for the confusion.
Flags: needinfo?(jmaher)
(Assignee)

Comment 6

5 years ago
Comment on attachment 822064 [details] [diff] [review]
Write disabled tests to the manifest and have the manifest parser report and skip them

(In reply to Joel Maher (:jmaher) from comment #5)
> Lets go with your original proposal, sorry for the confusion.

Was there anything else needed here?
Attachment #822064 - Flags: feedback- → feedback?(jmaher)
(Reporter)

Comment 7

5 years ago
Comment on attachment 822064 [details] [diff] [review]
Write disabled tests to the manifest and have the manifest parser report and skip them

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

the patch as it is seems good to me.
Attachment #822064 - Flags: feedback?(jmaher) → feedback+
(Assignee)

Comment 8

5 years ago
Comment on attachment 822064 [details] [diff] [review]
Write disabled tests to the manifest and have the manifest parser report and skip them

great! :)
Attachment #822064 - Flags: review?(jmaher)
(Reporter)

Comment 9

5 years ago
Comment on attachment 822064 [details] [diff] [review]
Write disabled tests to the manifest and have the manifest parser report and skip them

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

lets get this landed!
Attachment #822064 - Flags: review?(jmaher) → review+
(Assignee)

Comment 10

5 years ago
Thanks!

https://hg.mozilla.org/integration/mozilla-inbound/rev/48bfd1237326
Assignee: nobody → mhammond
Status: NEW → ASSIGNED
https://hg.mozilla.org/mozilla-central/rev/48bfd1237326
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
(Assignee)

Comment 12

5 years ago
Somehow I failed to push one of the chunks in this patch - which caused no problems, but didn't cause disabled tests to be passed to the JS runner.

I fixed this in a followup: https://hg.mozilla.org/integration/mozilla-inbound/rev/222711de6956
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
https://hg.mozilla.org/mozilla-central/rev/222711de6956
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.