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

Add default case for non-member types in assert_type_is #23754

Merged
merged 2 commits into from
May 28, 2020

Conversation

stephenmcgruer
Copy link
Contributor

Fixes #23346

@stephenmcgruer
Copy link
Contributor Author

Stability check failure was Firefox timing out. Results from wpt.fyi suggest no regressions, which is reasonable minimum bar.

Josh, Domenic - wdyt of this as a fix for #23346 ?

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.

The logic here LGTM. And if this causes no regressions then I'm happy to approve.

I worry a bit that without this, people will often forget to input important dependent IDL into their testharness tests. But probably the benefit in simple cases outweighs that.

resources/idlharness.js Outdated Show resolved Hide resolved
Copy link
Member

@inexorabletash inexorabletash left a comment

Choose a reason for hiding this comment

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

Hmmm... Is the set of intrinsic types from WebIDL enumerated anywhere?

For example, I see "Function" here but it's not listed anywhere in webidl2.js's source. It's a defined type in WebIDL that "coincidentally" has the same name in JS.

What I'm worried about: say someone writes an IDL with RegExp - a JS type but not a WebIDL type. Would that get caught by idlharness before (giving an error), but slip through now?

Approving though since this assert function is not where such things should get caught.

@inexorabletash
Copy link
Member

Following up to my previous comment: looks like idlharness indeed doesn't validate that types are known. Having a function return any of (Function, VoidFunction, NoSuchThing, RegExp) don't cause errors. Plenty of open issues against idlharness already, though.

Still LGTM, thanks!

@stephenmcgruer
Copy link
Contributor Author

Ok. I'm a bit hesitant here because the experts (y'all) are unsure (and I know so very little). But then again, experts also approved this so I guess its good :D.

Admin-merging because Firefox stability check is timing out due to number of tests affected.

@stephenmcgruer stephenmcgruer merged commit d94c7aa into master May 28, 2020
@stephenmcgruer stephenmcgruer deleted the smcgruer/idlharness branch May 28, 2020 17:38
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.

idlharness.js's assert_type_is needs to be taught about each interface
5 participants