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.

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!

The xpcshell harness already handles this, FWIW: http://hg.mozilla.org/mozilla-central/annotate/5a9ac6fed6ff/testing/xpcshell/runxpcshelltests.py#l529

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.

(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?

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.

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?

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.

Comment on attachment 822064 [details] [diff] [review] Write disabled tests to the manifest and have the manifest parser report and skip them great! :)

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!

Thanks! https://hg.mozilla.org/integration/mozilla-inbound/rev/48bfd1237326

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