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

test: separate the test fixtures between ICU and URL #35077

Closed
wants to merge 1 commit into from
Closed

test: separate the test fixtures between ICU and URL #35077

wants to merge 1 commit into from

Conversation

Leko
Copy link
Contributor

@Leko Leko commented Sep 6, 2020

We need to separate the fixtures between ICU and WPT URL to update the WPT fixtures. Since ICU and URL isn't the same implementation and they also follow different specifications. We shouldn't use the URL's fixture for ICU's toASCII.

For now, I duplicated the fixture to a new file.

Refs: #33770 (comment)

cc: @watilde

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

We need to emit dependency of ICU's toASCII in order to update the WPT
fixtures. Since ICU and URL isn't the same implementation and they also
follow different specifications. ICU's toASCII shouldn't have a
dependency on WPT fixtures.

Refs: #33770 (comment)
@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Sep 6, 2020
@watilde watilde added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 7, 2020
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 7, 2020
@nodejs-github-bot
Copy link
Collaborator

Copy link
Member

@watilde watilde left a comment

Choose a reason for hiding this comment

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

LGTM, thank you

@watilde
Copy link
Member

watilde commented Sep 8, 2020

Landed in 7913ac5

@watilde watilde closed this Sep 8, 2020
watilde pushed a commit that referenced this pull request Sep 8, 2020
We need to emit dependency of ICU's toASCII in order to update the WPT
fixtures. Since ICU and URL isn't the same implementation and they also
follow different specifications. ICU's toASCII shouldn't have a
dependency on WPT fixtures.

Refs: #33770 (comment)
PR-URL: #35077
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
@Leko Leko deleted the split-fixtures branch September 8, 2020 11:37
ruyadorno pushed a commit that referenced this pull request Sep 17, 2020
We need to emit dependency of ICU's toASCII in order to update the WPT
fixtures. Since ICU and URL isn't the same implementation and they also
follow different specifications. ICU's toASCII shouldn't have a
dependency on WPT fixtures.

Refs: #33770 (comment)
PR-URL: #35077
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
@ruyadorno ruyadorno mentioned this pull request Sep 21, 2020
4 tasks
joesepi pushed a commit to joesepi/node that referenced this pull request Jan 8, 2021
We need to emit dependency of ICU's toASCII in order to update the WPT
fixtures. Since ICU and URL isn't the same implementation and they also
follow different specifications. ICU's toASCII shouldn't have a
dependency on WPT fixtures.

Refs: nodejs#33770 (comment)
PR-URL: nodejs#35077
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants