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: add comment to all whatwg-url tests #12798

Closed
wants to merge 1 commit into from
Closed

test: add comment to all whatwg-url tests #12798

wants to merge 1 commit into from

Conversation

rayto510
Copy link

@rayto510 rayto510 commented May 2, 2017

A comment is added to the first line of each whatwg-url test stating
that they should not be modified.

Fixes: #12793

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
Affected core subsystem(s)

A comment is added to the first line of each whatwg-url test stating
that they should not be modified.

Fixes: #12793
@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label May 2, 2017
@refack
Copy link
Contributor

refack commented May 2, 2017

@rayto510 Thank.
BTW: You did have to open a new PR you could have edited the previous one.

@vsemozhetbyt
Copy link
Contributor

@mscdex mscdex added the whatwg-url Issues and PRs related to the WHATWG URL implementation. label May 2, 2017
@mscdex
Copy link
Contributor

mscdex commented May 2, 2017

Do all of these tests really require this comment? Perhaps we can just place the comment where the upstream tests are located?

@jasnell
Copy link
Member

jasnell commented May 2, 2017

I'd rather not do this either. If we do need to have it, then the comment should be expanded to explain why. It also should start with an upper case and have proper punctuation.

@gibfahn
Copy link
Member

gibfahn commented May 2, 2017

Do all of these tests really require this comment? Perhaps we can just place the comment where the upstream tests are located?

@mscdex AIUI the problem is that people may see them as simple test fixes and raise PRs, so if the comment isn't in that file then they'll probably end up getting modified.

@mscdex
Copy link
Contributor

mscdex commented May 2, 2017

@gibfahn I don't think it should be a problem if there is a clear boundary between upstream tests and our own.

Copy link
Member

@joyeecheung joyeecheung left a comment

Choose a reason for hiding this comment

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

So..to say whatwg url test should not be modified.
in all whatwg-url tests is not very accurate, because some tests are written by us and are not necessarily testing the spec behavior. Another approach would be to modify the the tests withWPT Refs: in the comment to something like The following tests are copied from WPT, modifications to them should be upstreamed first. Refs:

@gibfahn
Copy link
Member

gibfahn commented Jun 9, 2017

I don't think it should be a problem if there is a clear boundary between upstream tests and our own.

I don't think the fact that they all have the test-whatwg-url- prefix makes it obvious that they shouldn't be modified. I also don't see the harm in adding a comment to explain to people that any test changes here (presumably) should be upstreamed first. I guess if they went into a separate directory that might be enough, but I'd rather be explicit, it helps new collaborators and contributors at no great cost.


Another approach would be to modify the the tests withWPT Refs: in the comment to something like The following tests are copied from WPT, modifications to them should be upstreamed first. Refs:

Okay, so this only needs to be applied to files with WPT Refs: links. @rayto510 would you mind updating this PR? @joyeecheung's suggestion sounds good to me.

@gibfahn
Copy link
Member

gibfahn commented Jul 16, 2017

ping @rayto510

@rayto510 rayto510 closed this Jul 16, 2017
@refack refack self-assigned this Jul 16, 2017
@refack refack reopened this Jul 16, 2017
@refack
Copy link
Contributor

refack commented Jul 16, 2017

I'll follow up

@gautamarora
Copy link
Contributor

@refack Can i take a shot at this? Should i create a new PR?

@gautamarora
Copy link
Contributor

@refack i have created a new PR here: #14355

@gibfahn gibfahn closed this Jul 18, 2017
@refack refack removed their assignment Jul 20, 2017
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. whatwg-url Issues and PRs related to the WHATWG URL implementation.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Note that whatwg-url tests should not be modified
9 participants