Closed Bug 1265584 Opened 4 years ago Closed Last year

web-platform-tests don't report failures for unexpected NS_ASSERTIONs

Categories

(Testing :: web-platform-tests, defect)

defect
Not set

Tracking

(firefox62 fixed)

RESOLVED FIXED
mozilla62
Tracking Status
firefox62 --- fixed

People

(Reporter: dbaron, Assigned: jgraham)

References

(Blocks 1 open bug)

Details

Attachments

(10 files)

59 bytes, text/x-review-board-request
ahal
: review+
Details
59 bytes, text/x-review-board-request
ato
: review+
Details
59 bytes, text/x-review-board-request
ato
: review+
Details
59 bytes, text/x-review-board-request
ato
: review+
maja_zf
: review+
Details
59 bytes, text/x-review-board-request
maja_zf
: review+
Details
59 bytes, text/x-review-board-request
maja_zf
: review+
Details
59 bytes, text/x-review-board-request
maja_zf
: review+
Details
59 bytes, text/x-review-board-request
maja_zf
: review+
Details
59 bytes, text/x-review-board-request
maja_zf
: review+
Details
59 bytes, text/x-review-board-request
maja_zf
: review+
Details
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
Attachment #8976199 - Flags: review?(ato) → 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.
Attachment #8976200 - Flags: review?(ato) → 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?
Attachment #8976201 - Flags: review?(ato) → review-
Comment on attachment 8976198 [details]
Bug 1265584 - Fix logging of unexpected assertions with mach formatter,

https://reviewboard.mozilla.org/r/244372/#review250410
Attachment #8976198 - Flags: review?(ahalberstadt) → review+
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
Attachment #8976201 - Flags: review?(ato) → review+
Comment on attachment 8976201 [details]
Bug 1265584 - Add support for recording asserts in wptrunner,

https://reviewboard.mozilla.org/r/244378/#review251246
Attachment #8976201 - Flags: review?(mjzffr) → review+
Comment on attachment 8976202 [details]
Bug 1265584 - Reverse the order of metadata iteration,

https://reviewboard.mozilla.org/r/244380/#review251248
Attachment #8976202 - Flags: review?(mjzffr) → 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?
Attachment #8976203 - Flags: review?(mjzffr) → review+
Comment on attachment 8976204 [details]
Bug 1265584 - Use ujson where possible for faster metadata update,

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

Cool.
Attachment #8976204 - Flags: review?(mjzffr) → review+
Comment on attachment 8976205 [details]
Bug 1265584 - Output asserts to wptreport.json,

https://reviewboard.mozilla.org/r/244386/#review251264
Attachment #8976205 - Flags: review?(mjzffr) → review+
Comment on attachment 8976206 [details]
Bug 1265584 - Add a test for wpttest metadata,

https://reviewboard.mozilla.org/r/244388/#review251266
Attachment #8976206 - Flags: review?(mjzffr) → review+
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
Assignee: nobody → james
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
You need to log in before you can comment on or make changes to this bug.