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

Fix use of testharness.js/testharnessreport.js in *.window.js tests #36069

Merged
merged 1 commit into from
Sep 26, 2022

Conversation

foolip
Copy link
Member

@foolip foolip commented Sep 26, 2022

These scripts are already included in the template that turns
*.window.js into *.window.html. Including the twice leads to the error
"Identifier 'MessageQueue' has already been declared" in these tests
when run using wptrunner. MessageQueue is a class declared here:
https://github.com/web-platform-tests/wpt/blob/master/tools/wptrunner/wptrunner/testharnessreport.js

There's also one case of testharness-helpers.js being removed. That file
doesn't exist, and the assert_promise_rejects_with_message helper is
defined by /bluetooth/resources/bluetooth-test.js.

Add timeout=long for tests that took close to 10 seconds run and then
passed. These were flagged as slow tests by the Taskcluster stability
checks.

@foolip
Copy link
Member Author

foolip commented Sep 26, 2022

@WeizhongX FYI this is a case of a problem that only appears with wptrunner, so we haven't noticed this when developing tests in Chromium. Might be an example to add to "bad things that can happen when we don't use the same setup across CIs to run tests."

Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

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

LGTM, although it might be worth creating a lint rule for this at some point to prevent regressions.

@foolip
Copy link
Member Author

foolip commented Sep 26, 2022

I've filed #36071 for the lint.

@foolip
Copy link
Member Author

foolip commented Sep 26, 2022

So 4 of the tests take close to 10 seconds to run. navigation-helpers.window.html in both Chrome and Firefox, and 3 tests in /permissions-policy/experimental-features/ in just Chrome. I'll add // META: timeout=long to these tests.

These scripts are already included in the template that turns
*.window.js into *.window.html. Including the twice leads to the error
"Identifier 'MessageQueue' has already been declared" in these tests
when run using wptrunner. MessageQueue is a class declared here:
https://github.com/web-platform-tests/wpt/blob/master/tools/wptrunner/wptrunner/testharnessreport.js

There's also one case of testharness-helpers.js being removed. That file
doesn't exist, and the assert_promise_rejects_with_message helper is
defined by /bluetooth/resources/bluetooth-test.js.

Add timeout=long for tests that took close to 10 seconds run and then
passed. These were flagged as slow tests by the Taskcluster stability
checks.
@foolip foolip merged commit 5bcea26 into web-platform-tests:master Sep 26, 2022
@foolip foolip deleted the meta-testdriver branch September 26, 2022 20:00
@WeizhongX
Copy link
Contributor

@WeizhongX FYI this is a case of a problem that only appears with wptrunner, so we haven't noticed this when developing tests in Chromium. Might be an example to add to "bad things that can happen when we don't use the same setup across CIs to run tests."

Ack!

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.

6 participants