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

Update the encoding IDL file #9780

Merged

Conversation

5 participants
@lukebjerring
Copy link
Contributor

lukebjerring commented Mar 2, 2018

No description provided.

@wpt-pr-bot wpt-pr-bot requested review from domenic, jensl and yuki3 Mar 2, 2018

@wpt-pr-bot wpt-pr-bot added the encoding label Mar 7, 2018

@wpt-pr-bot wpt-pr-bot requested review from annevk and inexorabletash Mar 7, 2018

@foolip

foolip approved these changes Mar 7, 2018

Copy link
Contributor

foolip left a comment

Confirmed no interesting differences. Please review the idlharness.html change I've pushed.

@w3c-bots

This comment has been minimized.

Copy link

w3c-bots commented Mar 7, 2018

Build PASSED

Started: 2018-03-13 21:16:54
Finished: 2018-03-13 21:21:29

View more information about this build on:

Luke Bjerring

@lukebjerring lukebjerring force-pushed the lukebjerring:idl-file-updates-encoding branch from 77e3c12 to ab153b5 Mar 7, 2018

@inexorabletash

This comment has been minimized.

Copy link
Contributor

inexorabletash commented Mar 13, 2018

Why isn't encoding/idlharness.html updated to use this instead of inlining the IDL?

Or phrased differently: can you update the test to reference this, or explain why that's not appropriate here?

EDIT: Maybe that's @foolip's point... I'm not seeing the changes he pushed though. (Perhaps I just fail at github?)

@lukebjerring

This comment has been minimized.

Copy link
Contributor Author

lukebjerring commented Mar 13, 2018

You're right, it should, and Philip did, but the script blundered over the changes. @foolip can you dig them up and re-apply them. Sorry!

@foolip

This comment has been minimized.

Copy link
Contributor

foolip commented Mar 13, 2018

I found it in my reflog and pushed it again. @inexorabletash, can you review the change as a whole?

@inexorabletash
Copy link
Contributor

inexorabletash left a comment

Yay, lgtm.

We should make it an 'any.js' test and should probably scrub all the idlharness tests to figure out which actually need "untested" stuff but as long as this passes (it does, right?) then it's a good improvement - thanks!

@foolip

This comment has been minimized.

Copy link
Contributor

foolip commented Mar 14, 2018

as long as this passes (it does, right?)

Figuring that out is unfortunately not straightforward. From https://pulls.web-platform-tests.org/build/25890 it looks like no tests were run, but that's wrong, from the Travis logs we can see that they were. Filed web-platform-tests/pulls.web-platform-tests.org#62. And yes, the tests do work still, as far as eyeballing differences goes.

@foolip foolip merged commit b202bbb into web-platform-tests:master Mar 14, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.