Validate IDLs in idl_test() #18382

saschanaz · 2019-08-11T15:28:08Z

Related to #16267

Does not close the issue yet because the current deployed webidl2.js version is old.

resources/idlharness.js

saschanaz · 2019-08-16T09:20:14Z

@lukebjerring I expect any invalid IDL would immediately fail on CI, but this setup with idl_test fails only on actual WPT run. Do you think it would be good enough or is there a way to fail earlier?

Ms2ger · 2019-08-16T10:29:41Z

I guess we could try and set something up as part of / parallel to the lint, running over all the .idls in interfaces/. Maybe we should do both?

saschanaz · 2019-08-16T10:56:14Z

Being able to run any Node.js script on CI may help, as it seems everything is in Python now...

resources/idlharness.js

lukebjerring · 2019-08-16T13:58:35Z

resources/idlharness.js

+    return this.add_dependency_idls_parsed(WebIDL2.parse(raw_idls), options);
+};
+
+IdlArray.prototype.add_dependency_idls_parsed = function(parsed_idls, options)


This would be internal_add_dependency_idls if consistent with the add_idls method(s).

I don't like that name, but consistency is good.

resources/idlharness.js

lukebjerring · 2019-08-16T14:34:34Z

Seems like Taskcluster's not running any tests (for FF / Chrome).
I've opened #18500 to change that.

Looks like my changes to add the options params were bogus; I got mislead by GitHub's collapsed diff view. Prior to that, looks like quite the number (83) of regressions:
https://github.com/web-platform-tests/wpt/runs/193107226 shows https://wpt.fyi/results/?diff&filter=ADC&run_id=274850010&run_id=271080001 which, drilling down into the first regression, is

FAIL message: promise_test: Unhandled rejection with value: object "Error: Validation error at line 32 in IndexedDB, inside argument eventInitDict:
DOMString type, optional IDBVersionChangeEventInit eventInitDict)]
^ Optional dictionary arguments must have a default value of {}.

So, probably mostly the requirement of a default value?

resources/idlharness.js

saschanaz · 2019-08-16T14:41:25Z

Why are the GitHub Actions things always failing and sending tons of failure mails to me 🤔

lukebjerring · 2019-08-16T14:47:26Z

@jugglinmike might know. I noticed that started today too.

foolip

LGTM % nit

resources/idlharness.js

foolip · 2019-09-06T11:59:06Z

Here's a diff of full runs: https://wpt.fyi/results/?diff&filter=ADC&q=idlharness&run_id=306750001&run_id=315860003

And the affected tests: https://wpt.fyi/results/?diff&filter=ADC&run_id=275490002&run_id=306730001

Both make it look like this change will be fine.

My only concern is that it will make lots of idlharness.js tests that were previously entirely passing have one failing subtest, which will cause bugs to be automatically filed in at least Chromium. @Hexcles is there anything we need to do to prepare for that?

Hexcles · 2019-09-06T18:52:43Z

@foolip that's a lot of new failures... IIUC, this error indicates that something is wrong with the underlying IDL. Is the issue widespread among various IDLs, or are there a few common root causes that are referenced by a lot of other IDLs? It'd be great it's the latter and we could fix them before (or together with) landing this.

Hexcles · 2019-09-06T18:57:46Z

Also the error messages are really long, e.g.

FAIL message: Validation error at line 20 in BackgroundSync, inside `interface SyncEvent`:
[Constructor(DOMString type,
 ^ Constructors should now represented as constructor operations instead of `[Constructor]`. Refer to the [WebIDL spec section on constructor operations](https://heycam.github.io/webidl/#idl-constructors) for more information.

Validation error at line 158 in service-workers, inside `interface ExtendableEvent`:
[Constructor(DOMString type,
 ^ Constructors should now represented as constructor operations instead of `[Constructor]`. Refer to the [WebIDL spec section on constructor operations](https://heycam.github.io/webidl/#idl-constructors) for more information.

Validation error at line 167 in service-workers, inside `interface FetchEvent`:
[Constructor(DOMString type,
 ^ Constructors should now represented as constructor operations instead of `[Constructor]`. Refer to the [WebIDL spec section on constructor operations](https://heycam.github.io/webidl/#idl-constructors) for more information.

Validation error at line 186 in service-workers, inside `interface ExtendableMessageEvent`:
[Constructor(DOMString type,
 ^ Constructors should now represented as constructor operations instead of `[Constructor]`. Refer to the [WebIDL spec section on constructor operations](https://heycam.github.io/webidl/#idl-constructors) for more information.

Validation error at line 673 in html, inside `interface TrackEvent`:
 Constructor(DOMString type,
 ^ Constructors should now represented as constructor operations instead of `[Constructor]`. Refer to the [WebIDL spec section on constructor operations](https://heycam.github.io/webidl/#idl-constructors) for more information.

Validation error at line 1101 in html, inside `interface FormDataEvent`:
 Constructor(DOMString type,
 ^ Constructors should now represented as constructor operations instead of `[Constructor]`. Refer to the [WebIDL spec section on constructor operations](https://heycam.github.io/webidl/#idl-constructors) for more information.

Validation error at line 1398 in html, inside `interface ImageData`:
[Constructor(unsigned long sw
 ^ Constructors should now represented as constructor operations instead of `[Constructor]`. Refer to the [WebIDL spec section on constructor operations](https://heycam.github.io/webidl/#idl-constructors) for more information.

Validation error at line 1399 in html, inside `interface ImageData`:
 Constructor(Uint8ClampedArray data,
 ^ Constructors should now represented as constructor operations instead of `[Constructor]`. Refer to the [WebIDL spec section on constructor operations](https://heycam.github.io/webidl/#idl-constructors) for more information.

Validation error at line 1408 in html, inside `interface Path2D`:
[Constructor(optional (Path2D
 ^ Constructors should now represented as constructor operations instead of `[Constructor]`. Refer to the [WebIDL spec section on constructor operations](https://heycam.github.io/webidl/#idl-constructors) for more information.

Validation error at line 1434 in html, inside `interface OffscreenCanvas`:
[Constructor([EnforceRange]
 ^ Constructors should now represented as constructor operations instead of `[Constructor]`. Refer to the [WebIDL spec section on constructor operations](https://heycam.github.io/webidl/#idl-constructors) for more information.

Validation error at line 1526 in html, inside `interface DataTransfer`:
 Constructor]
 ^ Constructors should now represented as constructor operations instead of `[Constructor]`. Refer to the [WebIDL spec section on constructor operations](https://heycam.github.io/webidl/#idl-constructors) for more information.

Validation error at line 1564 in html, inside `interface DragEvent`:
 Constructor(DOMString type,
 ^ Constructors should now represented as constructor operations instead of `[Constructor]`. Refer to the [WebIDL spec section on constructor operations](https://heycam.github.io/webidl/#idl-constructors) for more information.

Validation error at line 1671 in html, inside `interface PopStateEvent`:
 Constructor(DOMString type,
 ^ Constructors should now represented as constructor operations instead of `[Constructor]`. Refer to the [WebIDL spec section on constructor operations](https://heycam.github.io/webidl/#idl-constructors) for more information.

Validation error at line 1681 in html, inside `interface HashChangeEvent`:
 Constructor(DOMString type,
 ^ Constructors should now represented as constructor operations instead of `[Constructor]`. Refer to the [WebIDL spec section on constructor operations](https://heycam.github.io/webidl/#idl-constructors) for more information.

Validation error at line 1693 in html, inside `interface PageTransitionEvent`:
 Constructor(DOMString type,
 ^ Constructors should now represented as constructor operations instead of `[Constructor]`. Refer to the [WebIDL spec section on constructor operations](https://heycam.github.io/webidl/#idl-constructors) for more information.

Validation error at line 1740 in html, inside `interface ErrorEvent`:
[Constructor(DOMString type,
 ^ Constructors should now represented as constructor operations instead of `[Constructor]`. Refer to the [WebIDL spec section on constructor operations](https://heycam.github.io/webidl/#idl-constructors) for more information.

Validation error at line 1758 in html, inside `interface PromiseRejectionEvent`:
[Constructor(DOMString type,
 ^ Constructors should now represented as constructor operations instead of `[Constructor]`. Refer to the [WebIDL spec section on constructor operations](https://heycam.github.io/webidl/#idl-constructors) for more information.

Validation error at line 2015 in html, inside `interface MessageEvent`:
[Constructor(DOMString type,
 ^ Constructors should now represented as constructor operations instead of `[Constructor]`. Refer to the [WebIDL spec section on constructor operations](https://heycam.github.io/webidl/#idl-constructors) for more information.

Validation error at line 2037 in html, inside `interface EventSource`:
[Constructor(USVString url,
 ^ Constructors should now represented as constructor operations instead of `[Constructor]`. Refer to the [WebIDL spec section on constructor operations](https://heycam.github.io/webidl/#idl-constructors) for more information.

Validation error at line 2060 in html, inside `interface WebSocket`:
[Constructor(USVString url,
 ^ Constructors should now represented as constructor operations instead of `[Constructor]`. Refer to the [WebIDL spec section on constructor operations](https://heycam.github.io/webidl/#idl-constructors) for more information.

Validation error at line 2089 in html, inside `interface CloseEvent`:
[Constructor(DOMString type,
 ^ Constructors should now represented as constructor operations instead of `[Constructor]`. Refer to the [WebIDL spec section on constructor operations](https://heycam.github.io/webidl/#idl-constructors) for more information.

Validation error at line 2103 in html, inside `interface MessageChannel`:
[Constructor, Exposed=(
 ^ Constructors should now represented as constructor operations instead of `[Constructor]`. Refer to the [WebIDL spec section on constructor operations](https://heycam.github.io/webidl/#idl-constructors) for more information.

Validation error at line 2125 in html, inside `interface BroadcastChannel`:
[Constructor(DOMString name)
 ^ Constructors should now represented as constructor operations instead of `[Constructor]`. Refer to the [WebIDL spec section on constructor operations](https://heycam.github.io/webidl/#idl-constructors) for more information.

Validation error at line 2175 in html, inside `interface Worker`:
[Constructor(USVString scriptURL,
 ^ Constructors should now represented as constructor operations instead of `[Constructor]`. Refer to the [WebIDL spec section on constructor operations](https://heycam.github.io/webidl/#idl-constructors) for more information.

Validation error at line 2196 in html, inside `interface SharedWorker`:
[Constructor(USVString scriptURL,
 ^ Constructors should now represented as constructor operations instead of `[Constructor]`. Refer to the [WebIDL spec section on constructor operations](https://heycam.github.io/webidl/#idl-constructors) for more information.

Validation error at line 2248 in html, inside `interface StorageEvent`:
 Constructor(DOMString type,
 ^ Constructors should now represented as constructor operations instead of `[Constructor]`. Refer to the [WebIDL spec section on constructor operations](https://heycam.github.io/webidl/#idl-constructors) for more information.

Validation error at line 6 in dom, inside `interface Event`:
[Constructor(DOMString type,
 ^ Constructors should now represented as constructor operations instead of `[Constructor]`. Refer to the [WebIDL spec section on constructor operations](https://heycam.github.io/webidl/#idl-constructors) for more information.

Validation error at line 48 in dom, inside `interface CustomEvent`:
[Constructor(DOMString type,
 ^ Constructors should now represented as constructor operations instead of `[Constructor]`. Refer to the [WebIDL spec section on constructor operations](https://heycam.github.io/webidl/#idl-constructors) for more information.

Validation error at line 60 in dom, inside `interface EventTarget`:
[Constructor,
 ^ Constructors should now represented as constructor operations instead of `[Constructor]`. Refer to the [WebIDL spec section on constructor operations](https://heycam.github.io/webidl/#idl-constructors) for more information.

Validation error at line 81 in dom, inside `interface AbortController`:
[Constructor,
 ^ Constructors should now represented as constructor operations instead of `[Constructor]`. Refer to the [WebIDL spec section on constructor operations](https://heycam.github.io/webidl/#idl-constructors) for more information.

Validation error at line 158 in dom, inside `interface MutationObserver`:
[Constructor(MutationCallback callback)
 ^ Constru…

Note that the example error ends with an ellipsis. It's not truncated by the wpt.fyi UI, but most likely by wptrunner and the actual message is even longer. This line will be duplicated in the results and baselines of all the affected idlharness tests.

foolip · 2019-09-06T19:06:58Z

Most of the validation errors are going to be for [Constructor] which should now be constructor(), missing default value {} for dictionaries, and perhaps missing [Exposed=Window].

@saschanaz could we filter out at least the first two cases in wpt and enable them once specs have been updated? I think we'll need this ability on an ongoing basis, as webidl2.js supports validating something before specs have updated, indeed as a way to find which specs need updating.

foolip · 2019-09-06T19:07:41Z

I want to also say that while the messages are verbose, it's a joy to see such clear error messages. That's certainly not always the case :)

saschanaz · 2019-09-07T03:38:38Z

@foolip I can certainly implement opt-out for those validations, but would be happier if we had a bot that automatically send a PR to fix those problems. Currently I have an autofixer https://github.com/saschanaz/webidl-updater/ but no bot.

foolip · 2019-09-07T07:40:41Z

A bot would be great, but even with the best of tooling I think you'll find that some specs require a lot of pinging to get anyone to review, so the whole process would be at least a few weeks. If we need to upgrade webidl2.js during a syntax transition, that would be an issue. But maybe we could do an ad-hoc filtering in those cases by string matching rather than a more robust opt-out supported by webidl2.js itself.

foolip · 2019-09-09T12:49:11Z

@saschanaz sending PRs automatically would be great, filed saschanaz/webidl-updater#1 with some tips.

Co-Authored-By: Philip Jägenstedt <philip@foolip.org>

saschanaz · 2019-09-24T14:05:01Z

Test run: https://wpt.fyi/results/?sha=d360c2abb8&max-count=1&product=safari

foolip · 2019-09-25T19:43:07Z

Do you know how many failing validate subtests there would be if this were landed now?

saschanaz · 2019-09-26T03:06:14Z

@foolip Based on https://wpt.fyi/results/?diff&filter=ADC&q=idlharness&run_id=331930007&run_id=331880009 I think it will be zero.

foolip · 2019-09-26T05:48:33Z

That diff doesn't include the new subtests even as passing, are you sure you compared the right two runs?

saschanaz · 2019-09-26T10:16:26Z

Oops, it seems something wrong was there. Got a new diff: https://wpt.fyi/results/?diff&filter=ADC&q=idlharness&run_id=331930007&run_id=324240015

It shows five failures:

Feature Policy: Remove [NoInterfaceObject] w3c/webappsec-permissions-policy#341
Media Session: Why is MediaPositionState nullable? w3c/mediasession#231
WebRTC (2 failures): Overloaded operations disallowed across partial interfaces in WebIDL w3c/webrtc-pc#2289
WebXR AR: Consider merging immersive-ar into WebXR immersive-web/webxr-ar-module#32. It redefines enum XRSessionMode to insert a new item and thus causes name collision with original definition of XRSessionMode in WebXR. As currently enums are not extendable, not sure what should be done, maybe the WebXR should just merge the new item?

lukebjerring · 2019-09-26T10:32:10Z

@saschanaz it sounds like this PR will make #12231 redundant then (namespace clashes)?

saschanaz · 2019-09-26T10:34:54Z

@lukebjerring Right, while this doesn't check member uniqueness yet.

foolip · 2019-09-26T12:08:37Z

That looks right, searching for seq(status:missing status:!pass) also gets those 5 failures, as well as some unrelated noise.

foolip · 2019-09-26T12:10:01Z

I believe those failures are few enough, and all have good issues filed, that we should merge this, to catch new problems as they trickle in.

foolip

validationIgnored mechanism looks great!

badolgardiola1997

master

wpt-pr-bot added idlharness.js infra labels Aug 11, 2019

wpt-pr-bot assigned jgraham Aug 11, 2019

wpt-pr-bot requested review from gsnedders and jgraham August 11, 2019 15:28

github-actions bot added the pull-request-has-preview label Aug 11, 2019

saschanaz force-pushed the webidl-validate branch 2 times, most recently from 7539f5d to e92ccf5 Compare August 12, 2019 05:18

Ms2ger reviewed Aug 13, 2019

View reviewed changes

resources/idlharness.js Outdated Show resolved Hide resolved

resources/idlharness.js Outdated Show resolved Hide resolved

saschanaz mentioned this pull request Aug 14, 2019

Update to webidl2@24.3.1 #18415

Merged

saschanaz force-pushed the webidl-validate branch from e92ccf5 to 98070dc Compare August 14, 2019 08:48

lukebjerring requested changes Aug 16, 2019

View reviewed changes

moz-wptsync-bot added mozilla:gecko-blocked and removed mozilla:gecko-blocked labels Aug 16, 2019

lukebjerring mentioned this pull request Aug 16, 2019

Run the results jobs when idlharness.js changes #18500

Merged

lukebjerring reviewed Aug 16, 2019

View reviewed changes

resources/idlharness.js Outdated Show resolved Hide resolved

lukebjerring reviewed Aug 16, 2019

View reviewed changes

resources/idlharness.js Outdated Show resolved Hide resolved

moz-wptsync-bot added mozilla:gecko-blocked and removed mozilla:gecko-blocked labels Aug 16, 2019

foolip approved these changes Sep 5, 2019

View reviewed changes

resources/idlharness.js Outdated Show resolved Hide resolved

moz-wptsync-bot removed the mozilla:gecko-blocked label Sep 5, 2019

saschanaz mentioned this pull request Sep 7, 2019

Support opt-out for validations w3c/webidl2.js#413

Closed

saschanaz and others added 3 commits September 24, 2019 21:47

Validate IDLs in idl_test()

c691f17

rename subtest

7f344a3

Co-Authored-By: Philip Jägenstedt <philip@foolip.org>

ignore errors that too frequent yet

d360c2a

saschanaz force-pushed the webidl-validate branch from 3fc49eb to d360c2a Compare September 24, 2019 12:48

saschanaz mentioned this pull request Sep 26, 2019

Consider merging immersive-ar into WebXR immersive-web/webxr-ar-module#32

Closed

foolip approved these changes Sep 26, 2019

View reviewed changes

foolip merged commit 0d9fecc into master Sep 26, 2019

foolip deleted the webidl-validate branch September 26, 2019 12:12

badolgardiola1997 reviewed Sep 26, 2019

View reviewed changes

saschanaz mentioned this pull request Sep 27, 2019

Allow partial enums whatwg/webidl#184

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Validate IDLs in idl_test() #18382

Validate IDLs in idl_test() #18382

saschanaz commented Aug 11, 2019

saschanaz commented Aug 16, 2019

Ms2ger commented Aug 16, 2019

saschanaz commented Aug 16, 2019

lukebjerring Aug 16, 2019

lukebjerring commented Aug 16, 2019

saschanaz commented Aug 16, 2019

lukebjerring commented Aug 16, 2019

foolip left a comment

foolip commented Sep 6, 2019

Hexcles commented Sep 6, 2019

Hexcles commented Sep 6, 2019

foolip commented Sep 6, 2019

foolip commented Sep 6, 2019

saschanaz commented Sep 7, 2019

foolip commented Sep 7, 2019

foolip commented Sep 9, 2019

saschanaz commented Sep 24, 2019

foolip commented Sep 25, 2019

saschanaz commented Sep 26, 2019

foolip commented Sep 26, 2019

saschanaz commented Sep 26, 2019 •

edited

Loading

lukebjerring commented Sep 26, 2019

saschanaz commented Sep 26, 2019

foolip commented Sep 26, 2019

foolip commented Sep 26, 2019

foolip left a comment

badolgardiola1997 left a comment

Validate IDLs in idl_test() #18382

Validate IDLs in idl_test() #18382

Conversation

saschanaz commented Aug 11, 2019

saschanaz commented Aug 16, 2019

Ms2ger commented Aug 16, 2019

saschanaz commented Aug 16, 2019

lukebjerring Aug 16, 2019

Choose a reason for hiding this comment

lukebjerring commented Aug 16, 2019

saschanaz commented Aug 16, 2019

lukebjerring commented Aug 16, 2019

foolip left a comment

Choose a reason for hiding this comment

foolip commented Sep 6, 2019

Hexcles commented Sep 6, 2019

Hexcles commented Sep 6, 2019

foolip commented Sep 6, 2019

foolip commented Sep 6, 2019

saschanaz commented Sep 7, 2019

foolip commented Sep 7, 2019

foolip commented Sep 9, 2019

saschanaz commented Sep 24, 2019

foolip commented Sep 25, 2019

saschanaz commented Sep 26, 2019

foolip commented Sep 26, 2019

saschanaz commented Sep 26, 2019 • edited Loading

lukebjerring commented Sep 26, 2019

saschanaz commented Sep 26, 2019

foolip commented Sep 26, 2019

foolip commented Sep 26, 2019

foolip left a comment

Choose a reason for hiding this comment

badolgardiola1997 left a comment

Choose a reason for hiding this comment

saschanaz commented Sep 26, 2019 •

edited

Loading