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 return value of DOMTokenList's replace() method #9920

Merged

Conversation

Projects
None yet
7 participants
@Zirro
Copy link
Member

Zirro commented Mar 8, 2018

This adds tests for a boolean value being returned from the replace() method, as discussed in whatwg/dom#577 and whatwg/dom#582.

It also adds a test which attempts to replace a token that isn't present with one that is, since this case was not covered by the existing tests.

@w3c-bots

This comment has been minimized.

Copy link

w3c-bots commented Mar 8, 2018

Build PASSED

Started: 2018-03-08 20:09:04
Finished: 2018-03-08 20:30:43

Failing Jobs

  • safari:11.0
  • MicrosoftEdge:16.16299

View more information about this build on:

@wpt-pr-bot wpt-pr-bot requested review from jensl and yuki3 Mar 8, 2018

@foolip
Copy link
Contributor

foolip left a comment

In the diff it looks like you've changed the indentation of many lines without changing anything else. Can you revert those changes to just have a minimal diff?

@Zirro

This comment has been minimized.

Copy link
Member Author

Zirro commented Mar 8, 2018

@foolip I think you're referring to the lines where I've added an additional null argument. I'm not sure why they're not being highlighted like the lines further down.

@foolip

This comment has been minimized.

Copy link
Contributor

foolip commented Mar 8, 2018

@Zirro, you're right, I didn't see that. I'll remove by request for changes, but also not reviewing this right now.

@foolip foolip dismissed their stale review Mar 8, 2018

Misread the diff

@@ -548,7 +548,7 @@ interface DOMTokenList {
[CEReactions] void add(DOMString... tokens);
[CEReactions] void remove(DOMString... tokens);
[CEReactions] boolean toggle(DOMString token, optional boolean force);
[CEReactions] void replace(DOMString token, DOMString newToken);
[CEReactions] boolean replace(DOMString token, DOMString newToken);

This comment has been minimized.

Copy link
@foolip

foolip Mar 8, 2018

Contributor

@lukebjerring, this is a problem we're going to run in pretty quickly with generated IDL, where people update the tests together with the spec. As a reviewer, I think we should probably just say "yes please" and if there are any differences the auto-update will take care of it. WDYT?

This comment has been minimized.

Copy link
@lukebjerring

lukebjerring Mar 12, 2018

Contributor

Presumably, differences would be subtle (whitespace, indenting, etc). Until the updater is mature, and we document how to run it, will just have to accept changes :)

@annevk

annevk approved these changes Mar 12, 2018

@annevk annevk merged commit 5a1c088 into web-platform-tests:master Mar 12, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

annevk added a commit to whatwg/dom that referenced this pull request Mar 12, 2018

Change DOMTokenList's replace() to return a boolean
The replace() method returns true if token was replaced with newToken, and false otherwise.

Tests: web-platform-tests/wpt#9920.

Fixes #577.

Zirro added a commit to Zirro/jsdom that referenced this pull request Mar 12, 2018

moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Mar 14, 2018

Bug 1444909 part 2. Change DOMTokenList.replace() to return a boolean…
… and pull in the corresponding web platform test updates. r=qdot

The wpt changes come from web-platform-tests/wpt#9920
and are needed to keep Element-classlist.html passing.

The change to testing/web-platform/mozilla/tests/dom/classList.html is pulling
those changes into our weird forked version of that test...

MozReview-Commit-ID: CvQlBRuieUY

mykmelez pushed a commit to mozilla/gecko that referenced this pull request Mar 15, 2018

Bug 1444909 part 2. Change DOMTokenList.replace() to return a boolean…
… and pull in the corresponding web platform test updates. r=qdot

The wpt changes come from web-platform-tests/wpt#9920
and are needed to keep Element-classlist.html passing.

The change to testing/web-platform/mozilla/tests/dom/classList.html is pulling
those changes into our weird forked version of that test...

MozReview-Commit-ID: CvQlBRuieUY

domenic added a commit to Zirro/jsdom that referenced this pull request Apr 1, 2018

domenic added a commit to jsdom/jsdom that referenced this pull request Apr 1, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.