David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla) Reporter
	Description • 8 years ago

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.

	Andreas Tolfsen ❲:ato❳
	Comment 11 • 6 years ago
mozreview-review

Comment on attachment 8976199 [details]
Bug 1265584 - Move wptrunner marionette usage onto a single thread,

https://reviewboard.mozilla.org/r/244374/#review250354

	Andreas Tolfsen ❲:ato❳
	Comment 12 • 6 years ago
mozreview-review

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.

	Andreas Tolfsen ❲:ato❳
	Comment 13 • 6 years ago
mozreview-review

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?

	Andrew Halberstadt [:ahal]
	Comment 14 • 6 years ago
mozreview-review

Comment on attachment 8976198 [details]
Bug 1265584 - Fix logging of unexpected assertions with mach formatter,

https://reviewboard.mozilla.org/r/244372/#review250410

	James Graham [:jgraham] Assignee
	Comment 15 • 6 years ago
mozreview-review-reply

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.

	Andreas Tolfsen ❲:ato❳
	Comment 26 • 6 years ago
mozreview-review-reply

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.

	Andreas Tolfsen ❲:ato❳
	Comment 27 • 6 years ago
mozreview-review

Comment on attachment 8976201 [details]
Bug 1265584 - Add support for recording asserts in wptrunner,

https://reviewboard.mozilla.org/r/244378/#review251062

	Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)
	Comment 28 • 6 years ago
mozreview-review

Comment on attachment 8976201 [details]
Bug 1265584 - Add support for recording asserts in wptrunner,

https://reviewboard.mozilla.org/r/244378/#review251246

	Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)
	Comment 29 • 6 years ago
mozreview-review

Comment on attachment 8976202 [details]
Bug 1265584 - Reverse the order of metadata iteration,

https://reviewboard.mozilla.org/r/244380/#review251248

	Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)
	Comment 30 • 6 years ago
mozreview-review

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?

	Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)
	Comment 31 • 6 years ago
mozreview-review

Comment on attachment 8976204 [details]
Bug 1265584 - Use ujson where possible for faster metadata update,

https://reviewboard.mozilla.org/r/244384/#review251262

Cool.

	Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)
	Comment 32 • 6 years ago
mozreview-review

Comment on attachment 8976205 [details]
Bug 1265584 - Output asserts to wptreport.json,

https://reviewboard.mozilla.org/r/244386/#review251264

	Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)
	Comment 33 • 6 years ago
mozreview-review

Comment on attachment 8976206 [details]
Bug 1265584 - Add a test for wpttest metadata,

https://reviewboard.mozilla.org/r/244388/#review251266

	Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)
	Comment 34 • 6 years ago
mozreview-review

Comment on attachment 8976207 [details]
Bug 1265584 - Update wpt metadata,

https://reviewboard.mozilla.org/r/244390/#review251268

	James Graham [:jgraham] Assignee
	Comment 35 • 6 years ago
mozreview-review-reply

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.

	Pulsebot
	Comment 36 • 6 years ago

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

	Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)
	Comment 37 • 6 years ago

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

	Cristina Coroiu [:ccoroiu]
	Comment 38 • 6 years ago
bugherder

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

	Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)
	Comment 39 • 6 years ago

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)

	Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)
	Comment 40 • 6 years ago

Upstream PR merged

	Pulsebot
	Comment 41 • 6 years ago

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

	Raul Gurzau (:RaulG)
	Comment 42 • 6 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/393fd99d44fa