test: add comments for whatwg-url tests #14355

gautamarora · 2017-07-18T19:59:52Z

Added comments to whatwg-url tests to document that they should not be modified until modifications are made upstream as per guidelines for Web Platform Tests

Fixes: #12793

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines
update: make lint

Affected core subsystem(s)

test, url

gibfahn

I like the format, not sure about the number of files modified.

gibfahn · 2017-07-18T23:29:20Z

test/parallel/test-whatwg-url-constructor.js

@@ -15,7 +15,8 @@ const request = {
 };

 /* eslint-disable */
-/* WPT Refs:
+/* The following tests are copied from WPT, modifications to them should be upstreamed first.


Looks like this PR only changes 16 files, but looking in test/parallel/ I see 26 with the test-whatwg prefix:

➜ parallel git:(master) ✗ ❯ ls | grep test-whatwg ~/wrk/com/node/test/parallel test-whatwg-url-constructor.js test-whatwg-url-domainto.js test-whatwg-url-historical.js test-whatwg-url-inspect.js test-whatwg-url-origin.js test-whatwg-url-parsing.js test-whatwg-url-properties.js test-whatwg-url-searchparams-append.js test-whatwg-url-searchparams-constructor.js test-whatwg-url-searchparams-delete.js test-whatwg-url-searchparams-entries.js test-whatwg-url-searchparams-foreach.js test-whatwg-url-searchparams-get.js test-whatwg-url-searchparams-getall.js test-whatwg-url-searchparams-has.js test-whatwg-url-searchparams-inspect.js test-whatwg-url-searchparams-keys.js test-whatwg-url-searchparams-set.js test-whatwg-url-searchparams-sort.js test-whatwg-url-searchparams-stringifier.js test-whatwg-url-searchparams-values.js test-whatwg-url-searchparams.js test-whatwg-url-setters.js test-whatwg-url-toascii.js test-whatwg-url-tojson.js test-whatwg-url-tostringtag.js ➜ parallel git:(master) ✗ ❯ ls | grep test-whatwg | wc -l ~/wrk/com/node/test/parallel 26

I believe the delta are files that do not contain imported code.

thanks for reviewing this @gibfahn. @refack is right, the other files for test-whatwg do not contain imported code, and infact have the comment Tests below are not from WPT.

refack · 2017-07-18T23:30:07Z

@gautamarora thank you very much. There are also 3 fixture files that would love to get some attention:

test\fixtures  (3 usages found)
                url-setter-tests.js  (1 usage found)
                    3 /* WPT Refs:
                url-tests.js  (1 usage found)
                    3 /* WPT Refs:
                url-toascii.js  (1 usage found)
                    3 /* WPT Refs:

gibfahn · 2017-07-18T23:30:38Z

Also thanks for being thorough, but could you remove these lines from the commit message?

##### Checklist
- [x] `make -j4 test` (UNIX), or `vcbuild test` (Windows) passes
- [x] commit message follows [commit
guidelines](https://github.com/nodejs/node/blob/master/CONTRIBUTING.md#c
ommit-message-guidelines)

##### Affected core subsystem(s)
test, url

refack

% adding the fixtures

refack

💯

gautamarora · 2017-07-19T00:23:01Z

@gibfahn - fixed body of commit.
@refack - added changes for text fixtures.

should the documentation for how to write tests for node.js also be updated to reflect this new format?

refack · 2017-07-19T00:25:08Z

should the documentation for how to write tests for node.js also be updated to reflect this new format?

That would be a nice to have:
https://github.com/nodejs/node/blob/master/doc/guides/writing-tests.md#web-platform-tests

gautamarora · 2017-07-19T00:37:08Z

@refack - updated the guide.

gautamarora · 2017-07-19T00:38:52Z

@refack - the body of the commit seems a bit messed up here, i'll try to fix this.

gautamarora · 2017-07-19T00:44:18Z

@refack - cleaned up the commit message. Let me know if there is any other clean up is needed.

vsemozhetbyt · 2017-07-19T00:46:24Z

CI: https://ci.nodejs.org/job/node-test-pull-request/9237/

TimothyGu · 2017-07-19T00:47:31Z

doc/guides/writing-tests.md

@@ -264,7 +264,8 @@ These imported tests will be wrapped like this:

 ```js
 /* eslint-disable */
-/* WPT Refs:
+/* The following tests are copied from WPT, modifications to them should be upstreamed first.


Can you please keep the lines under 80 chars? Not sure why the linter didn't catch that.

I can certainly make the change, the recommendation for that particular comment is as per discussion here cc @joyeecheung for feedback as well.

Maybe ignoreComments in max-len is true by default?

nevermind, i could also do something like this:

/* eslint-disable */ /* The following tests are copied from WPT, modifications to them should be upstreamed first. Refs: https://github.com/w3c/web-platform-tests/blob/54c3502d7b/url/urlsearchparams-constructor.html License: http://www.w3.org/Consortium/Legal/2008/04-testsuite-copyright.html */

making the changes now.

vsemozhetbyt · 2017-07-19T00:55:59Z

cc @not-an-aardvark, @silverwind, @Trott

Is ignoreComments in max-len true by default?
Is this option (omission) intentional in our main .eslintrc.yaml?

vsemozhetbyt · 2017-07-19T01:06:53Z

Seems false by default.
So linter misses max-len > 80 due to:

/* eslint-disable */
test/fixtures in the .eslintignore

Should we place /* eslint-disable */ after comments?

gautamarora · 2017-07-19T01:07:58Z

@TimothyGu - pushed a commit to keep lines under 80 chars in tests/fixtures/doc. let me know if this is good now.

not-an-aardvark · 2017-07-19T01:12:11Z

Is ignoreComments in max-len true by default?

No, it's false by default (i.e. the rule is strict by default on lines with comments, and allows you to opt out of checking them).

Is this option (omission) intentional in our main .eslintrc.yaml?

¯\_(ツ)_/¯

gibfahn · 2017-07-19T01:15:01Z

Should we place /* eslint-disable */ after comments?

Yes, I think we definitely should.

@gautamarora would you be able to do that?

vsemozhetbyt · 2017-07-19T01:15:56Z

@not-an-aardvark Thank you. Sorry for bothering, I've missed we eslintignore test/fixtures.

gautamarora · 2017-07-19T01:22:25Z

@gibfahn - yes, i will make the changes for this.

Added comments to whatwg-url tests that they should not be changed until modifications are merged upstream as per guidelines for [Web Platform Tests](https://github.com/nodejs/node/blob/master/doc/guides/writing-tests.md#web-platform-tests) Fixes: #12793

gautamarora · 2017-07-19T04:17:28Z

@gibfahn @vsemozhetbyt @TimothyGu

I've moved eslint: disable to after the comment in the test files. I also confirmed that once I moved this line to after the comment, the linter did fail my comments for being > 80. I've made the change in the guides as well.

For the test fixtures, the linting is ignored due to .eslintignore (as has been noted in earlier comments), but I have made sure that the new comments are within the 80 char limit.

vsemozhetbyt · 2017-07-19T04:52:57Z

CI: https://ci.nodejs.org/job/node-test-pull-request/9242/

Trott · 2017-07-19T05:03:57Z

Micro-nit: The first sentence of the comments contains a comma splice:

The following tests are copied from WPT, modifications to them should be upstreamed first.

The comma should be a period (or perhaps a semicolon):

The following tests are copied from WPT. Modifications to them should be upstreamed first.

No way this should hold up landing of the PR, though. :-D But if you want to fix it, or if you have to go in and fix other things anyway, then awesome.

gautamarora · 2017-07-19T05:10:57Z

@Trott - replaced comma with period :)

refack

💯+

refack · 2017-07-19T13:22:29Z

@gautamarora I see you are getting the full bikeshed PR treatment 😉. Thank you again for bearing with us...

gautamarora · 2017-07-19T15:24:21Z

@refack haha! no worries, i totally get it. Thanks for all the amazing feedback, i learnt much more than just adding comments like linting and running individual tests, so hopefully can contribute more. Let me know if you need anything more from my end.

From what I have understood so far, @vsemozhetbyt will need to start another CI build with the latest changes.

joyeecheung

LGTM. Thanks for picking this up!

joyeecheung · 2017-07-19T17:33:54Z

New CI: https://ci.nodejs.org/job/node-test-pull-request/9254/

refack · 2017-07-19T19:00:24Z

From what I have understood so far, vsemozhetbyt will need to start another CI build with the latest changes.

Anyone of the Collaborators can start a CI, you should ping us when you think the PR is stable.
The next lesson is patience, we keep PRs open at least 48 hours (72 on the weekends) so that everyone would get a chance to comment, as per https://github.com/nodejs/node/blob/master/COLLABORATOR_GUIDE.md#accepting-modifications
Also often change that include documentation received extra scrutiny, since we can't just test them, so human eyes are the only way to make sure they are as correct as possible.

Added comments to whatwg-url tests that they should not be changed until modifications are merged upstream as per "Web Platform Tests" guidelines PR-URL: nodejs#14355 Fixes: nodejs#12793 Refs: https://github.com/nodejs/node/blob/master/doc/guides/writing-tests.md Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Timothy Gu <timothygu99@gmail.com> Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com> Reviewed-By: James M Snell <jasnell@gmail.com>

refack · 2017-07-20T20:25:27Z

Landed in 9e4ab6c
@gautamarora You definatly got a warm welcome:

and now even GitHub recognizes you as:

Added comments to whatwg-url tests that they should not be changed until modifications are merged upstream as per "Web Platform Tests" guidelines PR-URL: #14355 Fixes: #12793 Refs: https://github.com/nodejs/node/blob/master/doc/guides/writing-tests.md Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Timothy Gu <timothygu99@gmail.com> Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com> Reviewed-By: James M Snell <jasnell@gmail.com>

nodejs-github-bot added the test Issues and PRs related to the tests. label Jul 18, 2017

gautamarora mentioned this pull request Jul 18, 2017

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

Closed

4 tasks

mscdex added the whatwg-url Issues and PRs related to the WHATWG URL implementation. label Jul 18, 2017

gibfahn requested review from joyeecheung, refack and vsemozhetbyt July 18, 2017 23:25

gibfahn reviewed Jul 18, 2017

View reviewed changes

refack approved these changes Jul 18, 2017

View reviewed changes

gibfahn approved these changes Jul 19, 2017

View reviewed changes

refack approved these changes Jul 19, 2017

View reviewed changes

refack self-assigned this Jul 19, 2017

TimothyGu suggested changes Jul 19, 2017

View reviewed changes

gautamarora added 3 commits July 19, 2017 00:02

test: fix to keep lines under 80 chars

7b63048

test: move eslint disable to after comments

7937fde

vsemozhetbyt approved these changes Jul 19, 2017

View reviewed changes

test: replacing comma with period in comments

7ae90c5

TimothyGu approved these changes Jul 19, 2017

View reviewed changes

refack approved these changes Jul 19, 2017

View reviewed changes

joyeecheung approved these changes Jul 19, 2017

View reviewed changes

lpinca approved these changes Jul 20, 2017

View reviewed changes

hiroppy approved these changes Jul 20, 2017

View reviewed changes

jasnell approved these changes Jul 20, 2017

View reviewed changes

gibfahn closed this Jul 21, 2017

gautamarora deleted the test-whatwg-url-comments branch July 21, 2017 10:28

addaleax mentioned this pull request Jul 24, 2017

v8.3.0 proposal #14452

Closed

addaleax mentioned this pull request Aug 2, 2017

v8.3.0 proposal #14594

Merged

MylesBorins added the dont-land-on-v6.x label Aug 16, 2017

refack removed their assignment Oct 20, 2018

test: add comments for whatwg-url tests #14355

test: add comments for whatwg-url tests #14355

Conversation

gautamarora commented Jul 18, 2017 • edited Loading

Checklist

Affected core subsystem(s)

gibfahn left a comment

Choose a reason for hiding this comment

gibfahn Jul 18, 2017

Choose a reason for hiding this comment

refack Jul 18, 2017 • edited Loading

Choose a reason for hiding this comment

gautamarora Jul 19, 2017

Choose a reason for hiding this comment

refack commented Jul 18, 2017

gibfahn commented Jul 18, 2017

refack left a comment • edited Loading

Choose a reason for hiding this comment

refack left a comment

Choose a reason for hiding this comment

gautamarora commented Jul 19, 2017

refack commented Jul 19, 2017

gautamarora commented Jul 19, 2017

gautamarora commented Jul 19, 2017

gautamarora commented Jul 19, 2017 • edited Loading

vsemozhetbyt commented Jul 19, 2017

TimothyGu Jul 19, 2017

Choose a reason for hiding this comment

gautamarora Jul 19, 2017

Choose a reason for hiding this comment

vsemozhetbyt Jul 19, 2017

Choose a reason for hiding this comment

gautamarora Jul 19, 2017

Choose a reason for hiding this comment

vsemozhetbyt commented Jul 19, 2017 • edited Loading

vsemozhetbyt commented Jul 19, 2017 • edited Loading

gautamarora commented Jul 19, 2017

not-an-aardvark commented Jul 19, 2017

gibfahn commented Jul 19, 2017

vsemozhetbyt commented Jul 19, 2017

gautamarora commented Jul 19, 2017

gautamarora commented Jul 19, 2017

vsemozhetbyt commented Jul 19, 2017

Trott commented Jul 19, 2017

gautamarora commented Jul 19, 2017

refack left a comment

Choose a reason for hiding this comment

refack commented Jul 19, 2017

gautamarora commented Jul 19, 2017

joyeecheung left a comment

Choose a reason for hiding this comment

joyeecheung commented Jul 19, 2017

refack commented Jul 19, 2017

refack commented Jul 20, 2017

gautamarora commented Jul 18, 2017 •

edited

Loading

refack Jul 18, 2017 •

edited

Loading

refack left a comment •

edited

Loading

gautamarora commented Jul 19, 2017 •

edited

Loading

vsemozhetbyt commented Jul 19, 2017 •

edited

Loading

vsemozhetbyt commented Jul 19, 2017 •

edited

Loading