Most of our test harnesses (e.g., reftest, crashtest, mochitest other than mochitest-browser-chrome) report test failures for firing of NS_ASSERTIONs that are not annotated as expected. This prevents regressions in NS_ASSERTIONS (which are assertions that shouldn't happen, but that we haven't been able to or don't want to make fatal). web-platform-tests doesn't do this. For example: Edit CanvasRenderingContext2D::DrawImage in dom/canvas/CanvasRenderingContext2D.cpp to start with: NS_ASSERTION(false, "have an assert"); rebuild (./mach build binaries), and then: ./mach web-platform-tests 2dcontext/drawing-images-to-the-canvas/drawimage_canvas_4.html There should be an unexpected failure reported, but instead the tests are reported as passing.

Comment on attachment 8976199 [details] Bug 1265584 - Move wptrunner marionette usage onto a single thread, https://reviewboard.mozilla.org/r/244374/#review250354

Comment on attachment 8976200 [details] Bug 1265584 - Make base_executor_kwargs arguments match executor_kwargs, https://reviewboard.mozilla.org/r/244376/#review250356 ::: testing/web-platform/tests/tools/wptrunner/wptrunner/browsers/chrome_android.py:47 (Diff revision 1) > server_config['ports']['http'] + server_config['ports']['https'] + > server_config['ports']['ws'] + server_config['ports']['wss'] > )) > > executor_kwargs = base_executor_kwargs(test_type, server_config, > - cache_manager, **kwargs) > + cache_manager, run_info_data,**kwargs) Space after comma.

Comment on attachment 8976201 [details] Bug 1265584 - Add support for recording asserts in wptrunner, https://reviewboard.mozilla.org/r/244378/#review250368 This seems like a good idea to keep track of. ::: testing/web-platform/tests/tools/wptrunner/wptrunner/executors/base.py:20 (Diff revision 1) > -def executor_kwargs(test_type, server_config, cache_manager, **kwargs): > +def executor_kwargs(test_type, server_config, cache_manager, run_info_data, > + **kwargs): Doesn’t this change belong in the previous patch? ::: testing/web-platform/tests/tools/wptrunner/wptrunner/executors/executormarionette.py:306 (Diff revision 1) > + > + def get(self): > + script = """ > + debug = Cc["@mozilla.org/xpcom/debug;1"].getService(Ci.nsIDebug2); > + if (debug.isDebugBuild) { > + return debug.assertionCount Missing semicolon. ::: testing/web-platform/tests/tools/wptrunner/wptrunner/executors/executormarionette.py:326 (Diff revision 1) > + except (errors.MarionetteException, socket.error): > + # This usually happens if the process crashed > + return None > + > + if self.parent.e10s: > + context_count = get_count("content", sandbox="system") You might not be in content context here. Wouldn’t it be safer to move the using_context context manager inside get_count? ::: testing/web-platform/tests/tools/wptrunner/wptrunner/executors/executormarionette.py:333 (Diff revision 1) > + if context_count is not None: > + count += context_count > + else: > + return The only reason I can tell for get_count returning None is that you want to return early from this overall function if failing to get the content count. Are you sure you don’t want to try getting the chrome assertion count? It doesn’t seem implausible that you might be able to get the chrome assertion count even if the content frame script doesn’t respond. Further, if you decide to do this you could get rid of the None return value from get_count, which means you could get rid of the if-else-conditions here. ::: testing/web-platform/tests/tools/wptrunner/wptrunner/executors/executormarionette.py:337 (Diff revision 1) > + context_count = get_count("chrome") > + if context_count is not None: > + count += context_count > + else: > + return > + if count: At this point count should always be >= 0? I don’t see the point of this if-else condition. ::: testing/web-platform/tests/tools/wptrunner/wptrunner/executors/executormarionette.py:587 (Diff revision 1) > self.do_testharness, > self.protocol, > self.test_url(test), > timeout).run() > + extra = None > + if self.debug and (success or data[0] not in ("CRASH", "INTERNAL-ERROR")): Can we assign data[0] to some more descriptive name?

Comment on attachment 8976198 [details] Bug 1265584 - Fix logging of unexpected assertions with mach formatter, https://reviewboard.mozilla.org/r/244372/#review250410

Comment on attachment 8976201 [details] Bug 1265584 - Add support for recording asserts in wptrunner, https://reviewboard.mozilla.org/r/244378/#review250368 > You might not be in content context here. Wouldn’t it be safer to > move the using_context context manager inside get_count? We certainly are, otherwise the entire rest of the program is going to fail. It doesn't seem worth the overhead of performing a no-op context switch just to make the code slightly cleaner.

Comment on attachment 8976201 [details] Bug 1265584 - Add support for recording asserts in wptrunner, https://reviewboard.mozilla.org/r/244378/#review250368 > We certainly are, otherwise the entire rest of the program is going to fail. It doesn't seem worth the overhead of performing a no-op context switch just to make the code slightly cleaner. OK.

Comment on attachment 8976201 [details] Bug 1265584 - Add support for recording asserts in wptrunner, https://reviewboard.mozilla.org/r/244378/#review251062

Comment on attachment 8976201 [details] Bug 1265584 - Add support for recording asserts in wptrunner, https://reviewboard.mozilla.org/r/244378/#review251246

Comment on attachment 8976202 [details] Bug 1265584 - Reverse the order of metadata iteration, https://reviewboard.mozilla.org/r/244380/#review251248

Comment on attachment 8976203 [details] Bug 1265584 - Support updating asserts with wpt-update, https://reviewboard.mozilla.org/r/244382/#review251252 ::: testing/web-platform/tests/tools/wptrunner/wptrunner/manifestupdate.py:450 (Diff revision 2) > + cls_default_value = 0 > + value_type = int > + > + def update_value(self, old_value, new_value): > + if old_value is not None: > + old_value = int(old_value) For consistency, how about using `value_type` here and in MaxAssertUpdate? ::: testing/web-platform/tests/tools/wptrunner/wptrunner/metadata.py:303 (Diff revision 2) > if data["status"] == "SKIP": > return > > result = test_cls.result_cls( > data["status"], > - data.get("message")) > + None) I must have missed something: why does `message` go away?

Comment on attachment 8976204 [details] Bug 1265584 - Use ujson where possible for faster metadata update, https://reviewboard.mozilla.org/r/244384/#review251262 Cool.

Comment on attachment 8976205 [details] Bug 1265584 - Output asserts to wptreport.json, https://reviewboard.mozilla.org/r/244386/#review251264

Comment on attachment 8976206 [details] Bug 1265584 - Add a test for wpttest metadata, https://reviewboard.mozilla.org/r/244388/#review251266

Comment on attachment 8976207 [details] Bug 1265584 - Update wpt metadata, https://reviewboard.mozilla.org/r/244390/#review251268

Comment on attachment 8976203 [details] Bug 1265584 - Support updating asserts with wpt-update, https://reviewboard.mozilla.org/r/244382/#review251252 > I must have missed something: why does `message` go away? Just because we never actually use the message for anything afaict. So it seems like a needless lookup to get it.

Pushed by james@hoppipolla.co.uk: https://hg.mozilla.org/integration/mozilla-inbound/rev/a6f3af165d48 Fix logging of unexpected assertions with mach formatter, r=ahal https://hg.mozilla.org/integration/mozilla-inbound/rev/9b8539a9a2a6 Move wptrunner marionette usage onto a single thread, r=ato https://hg.mozilla.org/integration/mozilla-inbound/rev/b99661f1fcc5 Make base_executor_kwargs arguments match executor_kwargs, r=ato https://hg.mozilla.org/integration/mozilla-inbound/rev/bc43ae3ccc9f Add support for recording asserts in wptrunner, r=ato, maja_zf https://hg.mozilla.org/integration/mozilla-inbound/rev/cc1d47bf0aa7 Reverse the order of metadata iteration, r=maja_zf https://hg.mozilla.org/integration/mozilla-inbound/rev/cc9b28394533 Support updating asserts with wpt-update, r=maja_zf https://hg.mozilla.org/integration/mozilla-inbound/rev/605324b9d70e Use ujson where possible for faster metadata update, r=maja_zf https://hg.mozilla.org/integration/mozilla-inbound/rev/b7716dedbf5a Output asserts to wptreport.json, r=maja_zf https://hg.mozilla.org/integration/mozilla-inbound/rev/b021a8615326 Add a test for wpttest metadata, r=maja_zf https://hg.mozilla.org/integration/mozilla-inbound/rev/4c4a74e7bdfd Update wpt metadata, r=maja_zf

Created web-platform-tests PR https://github.com/w3c/web-platform-tests/pull/11167 for changes under testing/web-platform/tests

https://hg.mozilla.org/mozilla-central/rev/a6f3af165d48 https://hg.mozilla.org/mozilla-central/rev/9b8539a9a2a6 https://hg.mozilla.org/mozilla-central/rev/b99661f1fcc5 https://hg.mozilla.org/mozilla-central/rev/bc43ae3ccc9f https://hg.mozilla.org/mozilla-central/rev/cc1d47bf0aa7 https://hg.mozilla.org/mozilla-central/rev/cc9b28394533 https://hg.mozilla.org/mozilla-central/rev/605324b9d70e https://hg.mozilla.org/mozilla-central/rev/b7716dedbf5a https://hg.mozilla.org/mozilla-central/rev/b021a8615326 https://hg.mozilla.org/mozilla-central/rev/4c4a74e7bdfd

Can't merge web-platform-tests PR due to failing upstream checks: Github PR https://github.com/w3c/web-platform-tests/pull/11167 * continuous-integration/travis-ci/pr (https://travis-ci.org/w3c/web-platform-tests/builds/383815393?utm_source=github_status&utm_medium=notification)

Upstream PR merged

Pushed by james@hoppipolla.co.uk: https://hg.mozilla.org/integration/mozilla-inbound/rev/393fd99d44fa [wpt PR 11167] - [Gecko Bug 1265584] Move wptrunner marionette usage onto a single thread, a=testonly