Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Do not try to format null assert statuses #27824

Closed
wants to merge 1 commit into from
Closed

Conversation

domenic
Copy link
Member

@domenic domenic commented Feb 28, 2021

This was causing the completion callback that the harness added,

function (tests, harness_status, asserts_run) {
   this_obj.output_handler.show_results(tests, harness_status, asserts_run);
}

to throw an exception, which would prevent all other completion callbacks from running, which would prevent jsdom from ever collecting any results.

@domenic
Copy link
Member Author

domenic commented Feb 28, 2021

There may be a better fix, e.g. not adding classes to null asserts, but this unblocks me, and I'd like to get it fixed ASAP (perhaps with someone else's better fix) because failing to call completion callbacks seems like a pretty bad contract violation.

@domenic domenic mentioned this pull request Feb 28, 2021
@stephenmcgruer
Copy link
Contributor

This can be reproduced by running html/semantics/scripting-1/the-script-element/execution-timing/020.html (e.g. http://wpt.live/html/semantics/scripting-1/the-script-element/execution-timing/020.html); thanks to Domenic for pointing it out.

I'm going to use that test to try and write a unittest for this PR today.

stephenmcgruer added a commit that referenced this pull request Mar 1, 2021
The assert tracking code cannot handle nested asserts. It ends up both
recording all the inner-asserts for assert_any (which is arguably
correct but also looks confusing since some fail), as well as not
properly recording the result for the `assert_any` itself (it overrides
the last inner-assert results instead - even more confusing!)

To workaround this quickly, do not wrap assert_any in the wrapper
script. This means it will not show up in the asserts run, though its
children will (but the last child will no longer have the wrong status).
This isn't perfect, but it's a quick fix.

Fixes #27824
@stephenmcgruer
Copy link
Contributor

Ah ok, so what is happening here is that assert_any is not well supported by the new assert tracking code - it cannot handle nested asserts.

I think what we need to do instead is to have assert_any bypass the wrapper for asserts it calls (maybe?) so that they don't get recorded, but in the meantime a quick fix is to disable the wrapper for assert_any; I'll do that.

jgraham pushed a commit that referenced this pull request Mar 2, 2021
The assert tracking code cannot handle nested asserts. It ends up both
recording all the inner-asserts for assert_any (which is arguably
correct but also looks confusing since some fail), as well as not
properly recording the result for the `assert_any` itself (it overrides
the last inner-assert results instead - even more confusing!)

To workaround this quickly, do not wrap assert_any in the wrapper
script. This means it will not show up in the asserts run, though its
children will (but the last child will no longer have the wrong status).
This isn't perfect, but it's a quick fix.

Fixes #27824
@domenic domenic deleted the domenic-patch-2 branch March 2, 2021 16:02
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Mar 15, 2021
…in expose_assert, a=testonly

Automatic update from web-platform-tests
[testharness.js] Do not wrap assert_any in expose_assert

The assert tracking code cannot handle nested asserts. It ends up both
recording all the inner-asserts for assert_any (which is arguably
correct but also looks confusing since some fail), as well as not
properly recording the result for the `assert_any` itself (it overrides
the last inner-assert results instead - even more confusing!)

To workaround this quickly, do not wrap assert_any in the wrapper
script. This means it will not show up in the asserts run, though its
children will (but the last child will no longer have the wrong status).
This isn't perfect, but it's a quick fix.

Fixes web-platform-tests/wpt#27824

--

wpt-commits: f84f16ed68aa6edaf5c98806e9d5c139a26882a6
wpt-pr: 27845
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this pull request Mar 15, 2021
…in expose_assert, a=testonly

Automatic update from web-platform-tests
[testharness.js] Do not wrap assert_any in expose_assert

The assert tracking code cannot handle nested asserts. It ends up both
recording all the inner-asserts for assert_any (which is arguably
correct but also looks confusing since some fail), as well as not
properly recording the result for the `assert_any` itself (it overrides
the last inner-assert results instead - even more confusing!)

To workaround this quickly, do not wrap assert_any in the wrapper
script. This means it will not show up in the asserts run, though its
children will (but the last child will no longer have the wrong status).
This isn't perfect, but it's a quick fix.

Fixes web-platform-tests/wpt#27824

--

wpt-commits: f84f16ed68aa6edaf5c98806e9d5c139a26882a6
wpt-pr: 27845

UltraBlame original commit: ce07f752bf60169ada7502badf8dd33ff9b642aa
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this pull request Mar 15, 2021
…in expose_assert, a=testonly

Automatic update from web-platform-tests
[testharness.js] Do not wrap assert_any in expose_assert

The assert tracking code cannot handle nested asserts. It ends up both
recording all the inner-asserts for assert_any (which is arguably
correct but also looks confusing since some fail), as well as not
properly recording the result for the `assert_any` itself (it overrides
the last inner-assert results instead - even more confusing!)

To workaround this quickly, do not wrap assert_any in the wrapper
script. This means it will not show up in the asserts run, though its
children will (but the last child will no longer have the wrong status).
This isn't perfect, but it's a quick fix.

Fixes web-platform-tests/wpt#27824

--

wpt-commits: f84f16ed68aa6edaf5c98806e9d5c139a26882a6
wpt-pr: 27845

UltraBlame original commit: ce07f752bf60169ada7502badf8dd33ff9b642aa
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this pull request Mar 15, 2021
…in expose_assert, a=testonly

Automatic update from web-platform-tests
[testharness.js] Do not wrap assert_any in expose_assert

The assert tracking code cannot handle nested asserts. It ends up both
recording all the inner-asserts for assert_any (which is arguably
correct but also looks confusing since some fail), as well as not
properly recording the result for the `assert_any` itself (it overrides
the last inner-assert results instead - even more confusing!)

To workaround this quickly, do not wrap assert_any in the wrapper
script. This means it will not show up in the asserts run, though its
children will (but the last child will no longer have the wrong status).
This isn't perfect, but it's a quick fix.

Fixes web-platform-tests/wpt#27824

--

wpt-commits: f84f16ed68aa6edaf5c98806e9d5c139a26882a6
wpt-pr: 27845

UltraBlame original commit: ce07f752bf60169ada7502badf8dd33ff9b642aa
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants