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

[streams] Refactor to multi-global test format #14172

Merged
merged 9 commits into from
Nov 22, 2018

Conversation

MattiasBuelens
Copy link
Contributor

@MattiasBuelens MattiasBuelens commented Nov 21, 2018

This changes all tests for streams to the multi-global .any.js format, and removes the old test wrappers.

Previously, the streams tests had to use custom test wrappers, because one of the tools used by the spec authors did not support auto-generating the necessary test boilerplate. This has been fixed in domenic/wpt-runner#12, so we can finally migrate the tests to the standard format. 😄

Fixes #10597

@MattiasBuelens
Copy link
Contributor Author

Hmm, I guess I should have run wpt lint first. Travis complains about lint errors:

ERROR:lint:streams/piping/error-propagation-forward.any.js:143: setTimeout used; step_timeout should typically be used instead (SET TIMEOUT)

I guess we can no longer use setTimeout inside tests? 😕

@jgraham
Copy link
Contributor

jgraham commented Nov 21, 2018

Presumably you moved files and a rule in lint.whitelist that previously matched now doesn't. But it would be good to remove the setTimeout use if possible.

@MattiasBuelens
Copy link
Contributor Author

@jgraham You're right, the paths changed, so I had to update the whitelist.

I'll see if switching to step_timeout "just works", then we can probably remove these from the whitelist. 🤞

@MattiasBuelens
Copy link
Contributor Author

Good news, it just works! 😄

Copy link
Contributor

@ricea ricea left a comment

Choose a reason for hiding this comment

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

lgtm

You no longer need the call to done() at the bottom of each js file. But it's not doing any harm.

streams/piping/close-propagation-forward.any.js Outdated Show resolved Hide resolved
@MattiasBuelens
Copy link
Contributor Author

Something went wrong with Taskcluster for Firefox Nightly? 😕 Looks like git fetch failed, see log:

++ git remote add origin https://github.com/web-platform-tests/wpt.git
++ git fetch --quiet --depth=50 origin refs/pull/14172/merge
fatal: The remote end hung up unexpectedly
[taskcluster 2018-11-22 10:26:23.174Z] === Task Finished ===
[taskcluster 2018-11-22 10:26:23.281Z] Unsuccessful task run with exit code: 128 completed in 3.829 seconds

@jgraham jgraham closed this Nov 22, 2018
@jgraham jgraham reopened this Nov 22, 2018
@jgraham
Copy link
Contributor

jgraham commented Nov 22, 2018

I closed and reopened as that's the only way to retrigger taskcluster until https://bugzilla.mozilla.org/show_bug.cgi?id=1509083 is fixed (reruns have complex semantics in general and people tried to deprecate them as a result, but they're what we need in this case, so…).

@MattiasBuelens
Copy link
Contributor Author

Rebased to fix conflicts with a51c2df.

@ricea ricea merged commit a89730a into web-platform-tests:master Nov 22, 2018
@MattiasBuelens MattiasBuelens deleted the streams-any-js branch November 22, 2018 15:44
aarongable pushed a commit to chromium/chromium that referenced this pull request Nov 27, 2018
With web-platform-tests/wpt#14172 wpt streams
tests now use the standard *.any.js format for multi-platform tests. As
a result all the test filenames have changed and there are a large
number of obsolete *-expected.txt files left behind. Delete them.

Change-Id: Ia5ce3b89e3866745d19bd1d521c7fb731342e024
Reviewed-on: https://chromium-review.googlesource.com/c/1350552
Reviewed-by: Yutaka Hirano <yhirano@chromium.org>
Commit-Queue: Adam Rice <ricea@chromium.org>
Cr-Commit-Position: refs/heads/master@{#611036}
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.

Clean up our custom multi-global solution for streams tests
5 participants