Skip to content

[infrastructure] Refactor tests & extend coverage#19916

Merged
jugglinmike merged 1 commit intoweb-platform-tests:masterfrom
bocoup:single-test-opt-in-infrastructure
Oct 30, 2019
Merged

[infrastructure] Refactor tests & extend coverage#19916
jugglinmike merged 1 commit intoweb-platform-tests:masterfrom
bocoup:single-test-opt-in-infrastructure

Conversation

@jugglinmike
Copy link
Contributor

@jugglinmike jugglinmike commented Oct 26, 2019

testharness.js was recently extended with an API to explicitly opt-in to
the "single page test" feature [1]. As per WPT RFC 28 [2], tests which
do not use this API and which do not declare any subtests will soon be
reported as a harness error.

Update some tests which previously opted in implicitly to use the new
API. Update others to instead declare a single subtest (so that they are
no longer single-page tests). Add new tests to fully cover the
conditions under which an uncaught exception or unhandled rejection
could occur.

[1] #19449
[2] https://github.com/web-platform-tests/rfcs/blob/master/rfcs/single_test.md

<script src="/resources/testharness.js"></script>
<script src="/resources/testharnessreport.js"></script>
<script>
setup({ single_test: true });
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just wrap this one in test(() => {})

<script src="/resources/testharness.js"></script>
<script src="/resources/testharnessreport.js"></script>
<script>
setup({ single_test: true });
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This and the following test was written to test when happens when there's a problem in unwrapped setup code. The expectations in infrastructure/metadata/ are what will have to change with the eventual change to remove implicit single-page tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Prior to the enforcement of single_test, there's no difference between "unwrapped setup code" and "failing single-page test code."

We'll need a new test for the "unwrapped setup code" when we begin enforcing single_test. Even then, we'll need to persist this test to preserve coverage for the "failing single-page test code" case.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having tests for both is fine, I'm just saying I want these to keep testing what they were, they're named appropriate for that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The names "uncaught exception" and "unhandled rejection" don't describe the tests' effect on the harness. Until we enforce single_test, those events will be interpreted as test failures.

I can add tests for harness errors, but these will be new tests. In order to demonstrate this today, they will have to define a sub-test before producing the uncaught exception or unhandled rejection.

A new commit on this branch introduces those tests.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The names "uncaught exception" and "unhandled rejection" don't describe the tests' effect on the harness.

Indeed, I'm following @zcorpan's style of describing what the test does rather than its pass condition in the naming, since the pass condition may change, and will change here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you revert the change to infrastructure/expected-fail/uncaught-exception.html?

@jugglinmike jugglinmike force-pushed the single-test-opt-in-infrastructure branch from 0a3aa41 to 491f4d3 Compare October 27, 2019 18:53
@jugglinmike jugglinmike changed the title [infrastructure] Opt-in to single-page test feature [infrastructure] Refactor tests Oct 27, 2019
@jugglinmike
Copy link
Contributor Author

Since the commit message would no longer describe this solution, I've amended the proposed commit and force-pushed the result to this branch. The original version of this branch is available here.

<script src="/resources/testharness.js"></script>
<script src="/resources/testharnessreport.js"></script>
<script>
setup({ single_test: true });
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like this change to be reverted too. Another test that looks like this would be fine.

@jugglinmike
Copy link
Contributor Author

@foolip This patch now includes the tests you requested.

@foolip
Copy link
Member

foolip commented Oct 30, 2019

Thanks @jugglinmike, looks great!

@foolip
Copy link
Member

foolip commented Oct 30, 2019

@jugglinmike not sure exactly what commit message you'd like, so feel free to merge yourself.

testharness.js was recently extended with an API to explicitly opt-in to
the "single page test" feature [1]. As per WPT RFC 28 [2], tests which
do not use this API and which do not declare any subtests will soon be
reported as a harness error.

Update some tests which previously opted in implicitly to use the new
API. Update others to instead declare a single subtest (so that they are
no longer single-page tests). Add new tests to fully cover the
conditions under which an uncaught exception or unhandled rejection
could occur.

[1] web-platform-tests#19449
[2] https://github.com/web-platform-tests/rfcs/blob/master/rfcs/single_test.md
@jugglinmike jugglinmike force-pushed the single-test-opt-in-infrastructure branch from 8018ac3 to 1ab620e Compare October 30, 2019 23:06
@jugglinmike jugglinmike changed the title [infrastructure] Refactor tests [infrastructure] Refactor tests & extend coverage Oct 30, 2019
@jugglinmike
Copy link
Contributor Author

Thanks, @foolip! I've squashed the commits, modified the commit message, force-pushed the result, and updated the pull request description accordingly. The prior version of this branch is available here.

Merging presently.

@jugglinmike jugglinmike merged commit b00f698 into web-platform-tests:master Oct 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants