Skip to content

Conversation

@rayto510
Copy link

@rayto510rayto510 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-botnodejs-github-bot added the test Issues and PRs related to the tests. label May 2, 2017
@refack
Copy link
Contributor

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

@vsemozhetbyt
Copy link
Contributor

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

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

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

@joyeecheungjoyeecheung 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

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

ping @rayto510

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

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

@gibfahngibfahn closed this Jul 18, 2017
@refackrefack removed their assignment Jul 20, 2017
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

testIssues and PRs related to the tests.whatwg-urlIssues 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

@rayto510@refack@vsemozhetbyt@mscdex@jasnell@gibfahn@gautamarora@joyeecheung@nodejs-github-bot