-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Improve reflected attribute tests (html/dom/reflection.js) #44355
Conversation
Without a toString:null in test values that are objects with a valueOf method, the DOM attribute would actually be set to "[object Object]" without ever calling the valueOf method (effectively turning those tests into behaving exactly the same as the {"test": 6} ones). Most existing tests were passing because the implemented algorithms would actually compute a WRONG expected value. The only test that doesn't compute the expected value ("double") won't actually run because all reflected IDL attributes of type "double" have custom getters. Fixes web-platform-tests#44315
Setting -0 to the IDL attribute will set the DOM attribute to "0" (per ECMAScript's ToString, from the rules for the the best representation of a number as a floating point number), and the rules for parsing flowting-point number values won't ever turn that "0" into a -0 value. This test value is never actually being used though, as all the reflected attributes of type "double" have custom getters.
Various improvements, each in its own commit. Let me know if you'd prefer separate PRs. |
Specifically leading non-ASCII whitespace following leading ASCII whitespace (should be rejected), and trailing non-ASCII whitespace (should be ignored)
Specifically "+" and "-" to exercise all possible code paths of failures.
I ran the tests with coverage (a copy, in a different project that reuses them), which led me to add a few tests (including exponential notation) and revealed a bug in the PR updated with the new tests and fixed harness. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally looks good, thanks! parseFloat
I think matches the spec except for infinities. (Maybe the spec should have this structure instead of goto-style.)
This should make it easier to extend tests with more values.
…ive values on setting. r=dom-core,edgar Tests will be in WPT (web-platform-tests/wpt#44355) Differential Revision: https://phabricator.services.mozilla.com/D200619
…ive values on setting. r=dom-core,edgar Tests will be in WPT (web-platform-tests/wpt#44355) Differential Revision: https://phabricator.services.mozilla.com/D200619
…ive values on setting. r=dom-core,edgar Tests will be in WPT (web-platform-tests/wpt#44355) Differential Revision: https://phabricator.services.mozilla.com/D200619 UltraBlame original commit: e62f150c6ffe94380dd9cd83d6058143065dea2c
…ive values on setting. r=dom-core,edgar Tests will be in WPT (web-platform-tests/wpt#44355) Differential Revision: https://phabricator.services.mozilla.com/D200619 UltraBlame original commit: e62f150c6ffe94380dd9cd83d6058143065dea2c
Fixes #44315