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

Handle options for add_untested_idls #10100

Conversation

Projects
None yet
4 participants
@lukebjerring
Copy link
Contributor

lukebjerring commented Mar 19, 2018

Currently, no way to import a subset of an IDL snippet as untested interfaces.

Note that the order of checking for should_skip needs to change for a case such as:
IDL:

interface A {}
interface B {}

let text = await fetch('interfaces/foo.idl').then(r => r.text());
idlArray.add_untested_idls(text, {only: ['A'] });
idlArray.add_idls(text, {only: ['B'] });

The add_idls encounters 'A' for a second time, and throws Duplicate identifier, in spite of the fact that it will be skipped.


This change is Reviewable

@lukebjerring lukebjerring requested a review from foolip Mar 19, 2018

@wpt-pr-bot wpt-pr-bot added the infra label Mar 19, 2018

@wpt-pr-bot wpt-pr-bot requested review from ayg, gsnedders and jgraham Mar 19, 2018

@w3c-bots

This comment has been minimized.

Copy link

w3c-bots commented Mar 19, 2018

Build PASSED

Started: 2018-03-22 15:09:37
Finished: 2018-03-22 15:15:19

View more information about this build on:

@foolip

foolip approved these changes Mar 20, 2018

@foolip

This comment has been minimized.

Copy link
Contributor

foolip commented Mar 20, 2018

The new failures are surprising and are probably not due to the change itself, but can you take a look?

@lukebjerring

This comment has been minimized.

Copy link
Contributor Author

lukebjerring commented Mar 22, 2018

Those same failures seem unrelated and currently occur running tox tests under resources/test/ in master, so we'll have to fix them.

@foolip

This comment has been minimized.

Copy link
Contributor

foolip commented Mar 22, 2018

Hmm, this is weird, no failures from https://travis-ci.org/w3c/web-platform-tests/jobs/356918923.

@lukebjerring, I guess you're also seeing failures on master locally? Seems like we might have two different issues going on here, both a difference between local runs and Travis, and some actual change to the results as a consequence of this PR?

@foolip

This comment has been minimized.

Copy link
Contributor

foolip commented Mar 22, 2018

Just to make sure, I'll rebase this on top of the same commit as #10145 to confirm the new failure.

@foolip foolip force-pushed the lukebjerring:idlharness-untested-idl-options branch from 60c26a6 to 988503c Mar 22, 2018

@foolip

This comment has been minimized.

Copy link
Contributor

foolip commented Mar 22, 2018

Well that's interesting, the problem was presumably on master but now fixed...

@lukebjerring, is that a satisfactory "explanation" of what's going on, or do you think we should dig deeper into what's going on before merging?

@lukebjerring

This comment has been minimized.

Copy link
Contributor Author

lukebjerring commented Mar 22, 2018

Well, we can conclude that this PR is not causing the breakage, but something fishy is going on with those test.

@lukebjerring lukebjerring merged commit df2e803 into web-platform-tests:master Mar 22, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@lukebjerring lukebjerring deleted the lukebjerring:idlharness-untested-idl-options branch Mar 22, 2018

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.