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

[WebIDL] add tests for is an array index abstract operation #9446

Closed

Conversation

tobie
Copy link
Contributor

@tobie tobie commented Feb 8, 2018

References whatwg/webidl#517.

@ghost
Copy link

ghost commented Feb 8, 2018

Build PASSED

Started: 2018-02-08 23:43:25
Finished: 2018-02-08 23:51:33

Failing Jobs

  • MicrosoftEdge:16.16299

Unstable Results

Browser: "Microsoftedge 16.16299" (failures allowed)

View in: WPT PR Status | TravisCI

Test Subtest Results Messages
/WebIDL/ecmascript-binding/legacy-platform-object.html   TIMEOUT: 1
OK: 9
  [[GetOwnProperty]] with getters and no setters FAIL: 9
assert_equals: (assert_prop_desc_equals: property 'writable') expected false but got true
  [[GetOwnProperty]] with indexed property getters and setters PASS: 9
  [[GetOwnProperty]] with named property getters and setters PASS: 9
  Test [[DefineOwnProperty]] with indexed property setter support. FAIL: 9
assert_throws: function "() => Object.defineProperty(select, "0", {get: () => {}})" did not throw
  Test [[DefineOwnProperty]] with no indexed property setter support. FAIL: 9
assert_equals: (assert_prop_desc_equals: property 'writable') expected false but got true
  Test 'is an array index' abstract operation. FAIL: 9
Unable to get property 'id' of undefined or null reference

@TimothyGu
Copy link
Member

I'd rather test this specifically for document.all, in which case a lot of these tests are already included:

https://github.com/w3c/web-platform-tests/blob/0c874a2e11885dc750902ae6c119f20affc6b49e/html/infrastructure/common-dom-interfaces/collections/htmlallcollection.html#L93-L100

@foolip
Copy link
Member

foolip commented Feb 9, 2018

Tests for document.all are in #9303

@foolip
Copy link
Member

foolip commented Feb 9, 2018

Don't review or merge it though, it's an automatic export!

@tobie tobie mentioned this pull request Feb 9, 2018
@domenic
Copy link
Member

domenic commented Feb 9, 2018

@TimothyGu why is testing document.all specifically a good idea here? I'd rather test a less-crazy object in the WebIDL/ tests, leaving document.all tests to a dedicated file.

@TimothyGu
Copy link
Member

@domenic It’s not, I agree with you. But this PR currently tests document.all.

@domenic
Copy link
Member

domenic commented Feb 9, 2018

Hmm, I guess I misunderstood your comment then, OK.

@tobie
Copy link
Contributor Author

tobie commented Feb 9, 2018

I’m fine testing using another object. Which one would you suggest that allows testing the same things?

@domenic
Copy link
Member

domenic commented Feb 9, 2018

Anything with an array index, right? I guess you'd ideally test all the call sites: get/set/define/delete. Maybe one object with an indexed getter only, and one object with an indexed setter? So like, NodeList and Storage?

// https://heycam.github.io/webidl/#is-an-array-index
assert_equals(document.all["4892.123"].id, "4892.123",
"Numbers with decimal points are not treated as array indices");
assert_equals(document.all["-0"], undefined, "'-0' is special cased");
Copy link
Member

Choose a reason for hiding this comment

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

This is tested by #9303. Testing some other object that document.all would be nice.

@foolip
Copy link
Member

foolip commented Nov 22, 2019

@tobie is this something you'd like to finish if I do review, or should we close it?

@foolip foolip closed this May 18, 2020
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.

5 participants