Remove testharness.js test-level timeouts (often used for async_test)

[foolip]

See discussion at #11029 (comment)

https://chromium-review.googlesource.com/c/chromium/src/+/1061753 shows what tests currently use this timeout.

Steps to this issue:

• Remove the use of { timeout: 1234 } from all tests
• Run https://chromium-review.googlesource.com/c/chromium/src/+/1061753 through Chromium's CQ again (or Gecko's)
• Remove support for the timeout entirely.

@jgraham, plan SGTY?

[foolip]

What would remain of the Test constructors second argument properties is the "assert" member, AFAICT only used in the output. @jgraham, should we want to get rid of that as well?

[jgraham]

Yeah, I'm happy with all of that plan, including removing the assert support.

If you make a PR it should automatically run all those tests through the gecko CI.

[foolip]

I've sent https://chromium-review.googlesource.com/c/chromium/src/+/1070155 to see if anything uses that.

[foolip]

List of tests that need updating:

css/css-transitions/currentcolor-animation-001.html
css/css-values/calc-unit-analysis.html
css/css-variables/test_variable_legal_values.html
css/cssom/computed-style-001.html
css/cssom/inline-style-001.html
css/cssom/medialist-interfaces-001.html
css/cssom/medialist-interfaces-002.html
css/cssom/medialist-interfaces-003.html
css/cssom/medialist-interfaces-004.html
css/cssom/style-sheet-interfaces-001.html
css/cssom/style-sheet-interfaces-002.html
css/cssom-view/media-query-list-interface.xht
css/cssom-view/offsetParent_element_test.html
css/cssom-view/window-interface.xht
css/mediaqueries/test_media_queries.html
hr-time/monotonic-clock.any.html
hr-time/monotonic-clock.any.worker.html
navigation-timing/test_document_open.html
navigation-timing/test_navigate_within_document.html
navigation-timing/test_navigation_attributes_exist.html
navigation-timing/test_navigation_redirectCount_none.html
navigation-timing/test_navigation_type_backforward.html
navigation-timing/test_navigation_type_enums.html
navigation-timing/test_navigation_type_reload.html
navigation-timing/test_no_previous_document.html
navigation-timing/test_performance_attributes_exist.html
navigation-timing/test_performance_attributes_exist_in_object.html
navigation-timing/test_readwrite.html
navigation-timing/test_timing_attributes_exist.html
navigation-timing/test_timing_attributes_order.html
navigation-timing/test_timing_client_redirect.html
navigation-timing/test_timing_reload.html
navigation-timing/test_timing_server_redirect.html
navigation-timing/test_timing_xserver_redirect.html
navigation-timing/test_unique_performance_objects.html
requestidlecallback/basic.html
resource-timing/resource_TAO_cross_origin_redirect_chain.html
resource-timing/resource_TAO_match_origin.htm
resource-timing/resource_TAO_match_wildcard.htm
resource-timing/resource_TAO_multi.htm
resource-timing/resource_TAO_null.htm
resource-timing/resource_TAO_origin.htm
resource-timing/resource_TAO_origin_uppercase.htm
resource-timing/resource_TAO_space.htm
resource-timing/resource_TAO_wildcard.htm
resource-timing/resource_TAO_zero.htm
resource-timing/resource_cached.htm
resource-timing/resource_connection_reuse.html
resource-timing/resource_dynamic_insertion.html
resource-timing/resource_timing_cross_origin_redirect_chain.html
user-timing/measure.html
user-timing/measure_navigation_timing.html

[foolip]

It looks like these strings show up in their own column in the report. Just having the information in the test name or assertion message would work. Needing to pick one means this will require some human judgement to do.

[qiuzhong]

I'm interested in working on this issue.

@foolip, I didn't find any strings like {timeout: 1234} in the html tests above. How did you get this list?

[foolip]

@qiuzhong, that's great! Try git grep -P "timeout:\s*\d+" in the root of the repository, that matches 223 files, most of which look like this: async_test("getCurrentPosition success callback tests", {timeout:20000}). The first step is to update those tests to look like this instead: async_test("getCurrentPosition success callback tests").

[foolip]

It would be a good idea to split those changes into multiple PRs since otherwise a lot of reviewers will get pulled in. Maybe one PR for each of html, IndexedDB, websockets, mediacapture-streams and cookies, and then one for everything else?

Once we think all uses are gone, I'll run a change similar to https://chromium-review.googlesource.com/1070155 through Chromium's CQ to see if anything was missed. Then we can just delete timeouts.

[qiuzhong]

@foolip, thanks for your good advice. It's really a good idea to split this update into several PRs.

I tried the git grep -P "timeout:\s*\d+" command and the output list differ from the one you provided above. Anyway, I'll update these test cases in serveral PRs. Now I'll list the submodules of these tests:

• IndexedDB
• cookies
• cors
• css
• custom-elements
• encrypted-media
• eventsource
• geolocation-API
• html
• mediacapture-streams
• old-tests
• requestidlecallback
• resource-timing
• resources
• shadow-dom
• web-nfc
• webauthn
• webrtc
• websockets
• xhr

[foolip]

@qiuzhong when you create the PRs, can you link to this issue so that's it easy to find them all? Thanks!

[qiuzhong]

@foolip , sure. I'll add the link of this issue for every PR and update the list status above.

[qiuzhong]

@foolip, There're promise_tests used in cookies tests and they have the {timeout: 1234} parameter as well. Should I remove them all?

wpt/cookies/http-state/attribute-tests.html

Line 21 in 706ee77

setup({ explicit_timeout: true });

wpt/cookies/http-state/attribute-tests.html

Lines 54 to 56 in 706ee77

	promise_test(createCookieTest(t.file),
	t.file + " - " + t.name,
	{ timeout: 3000 });

[foolip]

@FHorschig wrote that test. Given the use of an explicit timeout, I suspect that if we remove the test-level timeouts, then if any test doesn't finish, then the whole test would fail when wptrunner stops waiting for it. (This is different from testharness.js internally reaching a timeout and telling wptrunner.)

Can you start by checking what the effect on the test is of removing both of the lines you quoted?

[qiuzhong]

@foolip , I tried to remove both lines and found the pass rate of affected tests didn't change.

[foolip]

The promise_test ones should be removed, the ones in setup should not.

[foolip]

(At least not to resolve this issue, perhaps they should separately, later.)

[foolip]

I've sent https://chromium-review.googlesource.com/c/chromium/src/+/1061753 to see what tests still need updating.

[foolip]

That job has finished and these are the tests that still need updating:

cookies/http-state/attribute-tests.html
cookies/http-state/charset-tests.html
cookies/http-state/chromium-tests.html
cookies/http-state/comma-tests.html
cookies/http-state/domain-tests.html
cookies/http-state/general-tests.html
cookies/http-state/mozilla-tests.html
cookies/http-state/name-tests.html
cookies/http-state/ordering-tests.html
cookies/http-state/path-tests.html
cookies/http-state/value-tests.html
css/css-transitions/before-DOMContentLoaded-001.html
css/css-transitions/before-load-001.html
css/css-transitions/changing-while-transition.html
css/css-transitions/detached-container-001.html
css/css-transitions/hidden-container-001.html
css/css-transitions/properties-value-002.html
css/css-transitions/properties-value-003.html
css/css-transitions/properties-value-auto-001.html
css/css-transitions/properties-value-implicit-001.html
css/css-transitions/properties-value-inherit-003.html
css/css-transitions/pseudo-elements-001.html
geolocation-API/getCurrentPosition_IDL.https.html
webrtc/simplecall.https.html
webrtc/simplecall-no-ssrcs.https.html
xhr/abort-during-upload.htm
xhr/send-redirect-bogus.htm

@qiuzhong do you have PRs out for all of those?

[foolip]

Looks like #14253 covers some, yay :)

[foolip]

@qiuzhong with your two most recent PRs merged, I think the tests in css/css-transitions/ are all that remain.

[qiuzhong]

Besides the css/css-transitions/, there are other tests in css/css-regions/ that need to remove the timeout. The timeout is passed as an argument so they escaped from grepping timeout:\s*\d+.

[qiuzhong]

@foolip , the tests you listed above should be all covered now.

[qiuzhong]

@foolip, you listed the steps to remove testharness.js test-level timeouts:

• Remove the use of { timeout: 1234 } from all tests
• Run https://chromium-review.googlesource.com/c/chromium/src/+/1061753 through Chromium's CQ again (or Gecko's)
• Remove support for the timeout entirely.

I think the first step was done. But I don't know the status of step 2 so I'm not sure if it's time to start step 3.

[foolip]

I've started another run of https://chromium-review.googlesource.com/c/chromium/src/+/1061753, but please go ahead with step 3 in parallel, we can check the status of that before merging the final PR.

[foolip]

https://chromium-review.googlesource.com/c/chromium/src/+/1061753 is all green now, so good to go.

[foolip]

Resolved by #14309. Thanks @qiuzhong!