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 tests for liveness of NodeLists/HTMLCollections #8993

Merged
merged 4 commits into from
Apr 12, 2018

Conversation

xiuqijix
Copy link
Contributor

@xiuqijix xiuqijix commented Jan 11, 2018

Add following tests (Fix #1505):

  • Node#childNodes
  • ParentNode#children
  • {Document,Element}#getElementsByTagName
  • {Document,Element}#getElementsByTagNameNS
  • {Document,Element}#getElementsByClassName
  • Document#images
  • Document#embeds
  • Document#plugins
  • Document#links
  • Document#forms
  • Document#scripts
  • Document#getElementsByName

@Ms2ger, PTAL.

@ghost
Copy link

ghost commented Jan 11, 2018

Build PASSED

Started: 2018-01-11 08:26:28
Finished: 2018-01-11 08:34:04

View more information about this build on:

@foolip
Copy link
Member

foolip commented Jan 11, 2018

From https://pulls.web-platform-tests.org/build/22635 it looks like all browsers pass all of these tests. That's a bit surprising, are things here really perfectly interoperable? @Ms2ger, when you filed #1505, were you aware of anything in particular in need of testing?

@gsnedders
Copy link
Member

@foolip for most of these, much of the web would be badly broken if they weren't live, so I'm not surprised by interop.

@Ms2ger
Copy link
Contributor

Ms2ger commented Jan 11, 2018

I don't recall if there were any bugs I knew of at that time; I think I would have said so if I did.

@xiuqijix
Copy link
Contributor Author

@Ms2ger, any comments on my tests?

@Ms2ger
Copy link
Contributor

Ms2ger commented Jan 15, 2018

Haven't had time to review them yet.

@plehegar
Copy link
Member

I wonder if it's worth testing childNodes with an innerHTML manipulation.

@foolip
Copy link
Member

foolip commented Jan 27, 2018

@tkent-google, is this something you could review?

Copy link
Contributor

@tkent-google tkent-google left a comment

Choose a reason for hiding this comment

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

I prefer adding liveness tests to each of API test files to adding a single liveness test file for various API.

e.g. The first test() in Node-collection-live.html should be in dom/nodes/Node-childNodes.html.

@xiuqijix
Copy link
Contributor Author

@tkent-google, sorry for late response, I have addressed your comments in new commit, PTAL.

 - Node#childNodes
 - ParentNode#children
 - {Document,Element}#getElementsByTagName
 - {Document,Element}#getElementsByTagNameNS
 - {Document,Element}#getElementsByClassName
 - Document#images
 - Document#embeds
 - Document#plugins
 - Document#links
 - Document#forms
 - Document#scripts
 - Document#getElementsByName
@@ -18,4 +18,23 @@
assert_false(list instanceof NodeList, "NodeList")
assert_true(list instanceof HTMLCollection, "HTMLCollection")
}, "Interface should be correct.")
test(function() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Add a blank line before this line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated


element.removeChild(t2);
assert_equals(l.length, 1);
}, "getElementsByTagName() return an HTMLCollection is liveness");
Copy link
Contributor

Choose a reason for hiding this comment

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

The test description looks weird grammatically.
Should it be "getElementsByTagName() should be a live collection" or something?

This comment should be applied to all of new tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, @tkent-google, updated the test descriptions.

@tkent-google tkent-google merged commit 2d4fbd4 into web-platform-tests:master Apr 12, 2018
@xiuqijix xiuqijix deleted the Fix_nodelist_live branch April 12, 2018 06:07
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.

7 participants