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: move WPT to its own testing module #12736

Closed
wants to merge 1 commit into from

Conversation

@Trott
Copy link
Member

commented Apr 29, 2017

This is first in a hoped-for series of moves away from a monolithic
common.js that is loaded for every test and towards a more modular
approach. (In the end, common.js will hopefully contain checks for
variables leaking into the global space and perhaps some of the more
ubiquitous functions like common.mustCall().)

Move the WPT testing code to its own module.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

test url

@Trott

This comment has been minimized.

Copy link
Member Author

commented Apr 29, 2017

This is a sort of trial balloon to see if there's support for this sort of change generally. /cc @nodejs/testing @nodejs/url

@Trott

This comment has been minimized.

Copy link
Member Author

commented Apr 29, 2017

@@ -226,109 +228,109 @@ Tests whether `name` and `expected` are part of a raised warning.

This comment has been minimized.

Copy link
@gibfahn

gibfahn Apr 29, 2017

Member

## getArrayBufferViews(buf) -> #### getArrayBufferViews(buf) right? (L225)

@targos

This comment has been minimized.

Copy link
Member

commented Apr 29, 2017

+1 on the initiative, but I wonder if we shouldn't have a test/lib/ directory to put those factored out modules.

@gibfahn

This comment has been minimized.

Copy link
Member

commented Apr 29, 2017

LGTM in this case as common.WPT seems always used separately, so

-const { test, assert_equals } = common.WPT;
+const { test, assert_equals } = require('../wpt');

isn't a big deal.

But in general if we're going to split up common and start having:

-const common = require('../common');
+require('../common');
+const commonx = require('../commonx');
+const commony = require('../commony');
+const commonz = require('../commonz');

then I'm not sure what the benefit is (or if it outweighs the increased complexity, especially for new contributors).


Same applies to adding a test/lib/ directory. At the moment anyone looking at test/ sees test folders and a common.js file, which I'd hope is pretty intuitive (and a style used by other projects). Of course we do already have test/testpy/, which is non-obvious. Maybe a test/common/ directory would be obvious enough, 🤷‍♀️ .

@targos

This comment has been minimized.

Copy link
Member

commented Apr 29, 2017

Maybe a test/common/ directory would be obvious enough

Good idea. We could move common.js to test/common/index.js in that case, keeping require('../common') compatibility.

@benjamingr
Copy link
Member

left a comment

I'm +0 on doing this but the changes LGTM

test/README.md Outdated
* return [<Boolean>](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Data_structures#Boolean_type)

Checks whether `IPv6` is supported on this platform.

### hasMultiLocalhost
#### hasMultiLocalhost

This comment has been minimized.

Copy link
@richardlau

richardlau Apr 29, 2017

Member

If we're editing these, could we move the h* properties so that they're in alphabetical order?

@cjihrig

This comment has been minimized.

Copy link
Contributor

commented Apr 29, 2017

I like the idea of a test/common/ directory to split up the existing large file.

I don't like losing access to everything I need for testing from require('../common'). What if we lazy loaded less commonly used things?

@mhdawson

This comment has been minimized.

Copy link
Member

commented May 1, 2017

I'm +0 zero on this change. Is it a problem to load it for every test ? If not then if we want to modularize into separate files maybe we can do that but still end up with a single import ?

@Trott Trott force-pushed the Trott:demonolith-wpt branch May 1, 2017

@Trott

This comment has been minimized.

Copy link
Member Author

commented May 1, 2017

rebased to resolve conflict and resolve README nits

@Trott

This comment has been minimized.

Copy link
Member Author

commented May 1, 2017

Even if we don't do this for anything else, I think it makes a lot of sense to split out the WPT stuff because it really is its own thing that gets used its own way. People shouldn't use it without fully understanding where it comes from and why it is the way it is. At the same time, it isn't something people need to bother with in the vast majority of cases.

@Fishrock123

This comment has been minimized.

Copy link
Member

commented May 1, 2017

Uhm, what is "WPT"?

@Trott

This comment has been minimized.

Copy link
Member Author

commented May 1, 2017

@TimothyGu

This comment has been minimized.

Copy link
Member

commented May 2, 2017

I'm indifferent towards splitting out WPT. Its (very specific) use is documented, and I'm of the opinion that people generally won't touch the stuff they don't know about. Though I can see how logically it might not fit with the rest of test/common.js.

On splitting up test/common.js in general, I'm –½. What are the benefits of splitting it up? This might sound snarky, but AFAICT the utilities can all go under one category "miscellaneous."

On splitting up and lazy-loading different utilities, I'm –1. If the time being spent on loading test/common/* is insignificant then lazy-loading would be overkill, and if not I'd advocate for not splitting it at all.

@Trott

This comment has been minimized.

Copy link
Member Author

commented May 2, 2017

@TimothyGu You make some good points, for sure. The main benefit would be for newcomers looking at the docs. Instead of dozens of properties, many of which won't matter to them, it can just be properties that are easy to understand and whose value is readily apparent. Like, common.isWindows? Sure, I get it. common.mustCall()? Yup, that's useful. Someone can read the common docs and feel like they've understood the module rather than puzzling over when, why, and how to use common.spawnSyncPwd() and common.arrayStream.

(On the other hand, looking at common, most of the things fall into the "ubiquitous, simple, and useful" category. But for the things that don't...)

@Trott Trott force-pushed the Trott:demonolith-wpt branch May 2, 2017

@Trott

This comment has been minimized.

Copy link
Member Author

commented May 2, 2017

I really like the common/ directory idea so I went ahead and did that....

@Trott

This comment has been minimized.

Copy link
Member Author

commented May 2, 2017

test/README.md Outdated
* return [<String>](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Data_structures#String_type)

Name of the temp directory used by tests.

### WPT
### wpt

This comment has been minimized.

Copy link
@richardlau

richardlau May 2, 2017

Member

Should this be common/wpt?

test/README.md Outdated
## Common module API
## Testing Module APIs

### common

The common.js module is used by tests for consistency across repeated

This comment has been minimized.

Copy link
@richardlau

richardlau May 2, 2017

Member

Are we still calling the module common.js after the refactor? How about just common?

@Trott Trott force-pushed the Trott:demonolith-wpt branch May 2, 2017

@Trott

This comment has been minimized.

Copy link
Member Author

commented May 2, 2017

Pushed fixes for README nits.

Probably want to move the docs for the common module etc. into test/common/README.md and provide a link in test/README.md?

@Trott Trott force-pushed the Trott:demonolith-wpt branch May 2, 2017

@Trott

This comment has been minimized.

Copy link
Member Author

commented May 2, 2017

Split out into separate READMEs and force-pushed.

@Trott

This comment has been minimized.

Copy link
Member Author

commented May 2, 2017

@Trott Trott force-pushed the Trott:demonolith-wpt branch May 3, 2017

@Trott

This comment has been minimized.

Copy link
Member Author

commented May 3, 2017

@richardlau

This comment has been minimized.

Copy link
Member

commented May 3, 2017

Probably want to move the docs for the common module etc. into test/common/README.md and provide a link in test/README.md?

The link is missing. Probably should add an entry for common under the test directories table.

@Trott

This comment has been minimized.

Copy link
Member Author

commented May 3, 2017

The link is missing. Probably should add an entry for common under the test directories table.

Heh! Also: Should add an entry for the common directory in the main README and put the link there (either in addition or instead).

@Trott Trott force-pushed the Trott:demonolith-wpt branch May 3, 2017

@Trott

This comment has been minimized.

Copy link
Member Author

commented May 3, 2017

Entry for common directory and link to that directory's README.md added to main test directory's README.

@Trott

This comment has been minimized.

Copy link
Member Author

commented May 3, 2017

test/README.md Outdated
@@ -49,6 +44,14 @@ On how to run tests in this direcotry, see
</td>
</tr>
<tr>
<td>common</td>
<td>No</td>

This comment has been minimized.

Copy link
@richardlau

richardlau May 3, 2017

Member

fixtures and testpy leave this column empty.

@richardlau
Copy link
Member

left a comment

Minor nit otherwise LGTM.

test: move WPT to its own testing module
This is first in a hoped-for series of moves away from a monolithic
common.js that is loaded for every test and towards a more modular
approach. (In the end, common.js will hopefully contain checks for
variables leaking into the global space and perhaps some of the more
ubiquitous functions like common.mustCall().)

Move the WPT testing code to its own module.

@Trott Trott force-pushed the Trott:demonolith-wpt branch to 4086e82 May 3, 2017

@Trott

This comment has been minimized.

Copy link
Member Author

commented May 3, 2017

@Trott

This comment has been minimized.

Copy link
Member Author

commented May 4, 2017

Landed in ff001c1

Trott added a commit to Trott/io.js that referenced this pull request May 4, 2017
test: move WPT to its own testing module
This is first in a hoped-for series of moves away from a monolithic
common.js that is loaded for every test and towards a more modular
approach. (In the end, common.js will hopefully contain checks for
variables leaking into the global space and perhaps some of the more
ubiquitous functions like common.mustCall().)

Move the WPT testing code to its own module.

PR-URL: nodejs#12736
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>

@Trott Trott closed this May 4, 2017

anchnk pushed a commit to anchnk/node that referenced this pull request May 6, 2017
test: move WPT to its own testing module
This is first in a hoped-for series of moves away from a monolithic
common.js that is loaded for every test and towards a more modular
approach. (In the end, common.js will hopefully contain checks for
variables leaking into the global space and perhaps some of the more
ubiquitous functions like common.mustCall().)

Move the WPT testing code to its own module.

PR-URL: nodejs#12736
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
@jasnell jasnell referenced this pull request May 11, 2017
@gibfahn gibfahn referenced this pull request Jun 15, 2017
2 of 3 tasks complete
@gibfahn

This comment has been minimized.

Copy link
Member

commented Jun 18, 2017

@Trott could you backport this to v6.x-staging? It'd be good to get this backported as otherwise this is going to cause a lot of conflicts.

If yes please follow the guide and raise a backport PR, if no let me know or add the dont-land-on label.

Trott added a commit to Trott/io.js that referenced this pull request Jun 19, 2017
test: move WPT to its own testing module
This is first in a hoped-for series of moves away from a monolithic
common.js that is loaded for every test and towards a more modular
approach. (In the end, common.js will hopefully contain checks for
variables leaking into the global space and perhaps some of the more
ubiquitous functions like common.mustCall().)

Move the WPT testing code to its own module.

PR-URL: nodejs#12736
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
@Trott Trott referenced this pull request Jun 19, 2017
0 of 4 tasks complete
@Trott

This comment has been minimized.

Copy link
Member Author

commented Jun 19, 2017

@gibfahn backport in #13775

gibfahn added a commit that referenced this pull request Jun 20, 2017
test: move WPT to its own testing module
This is first in a hoped-for series of moves away from a monolithic
common.js that is loaded for every test and towards a more modular
approach. (In the end, common.js will hopefully contain checks for
variables leaking into the global space and perhaps some of the more
ubiquitous functions like common.mustCall().)

Move the WPT testing code to its own module.

PR-URL: #12736
Backport-PR-URL: #13775
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
MylesBorins added a commit that referenced this pull request Jul 11, 2017
test: move WPT to its own testing module
This is first in a hoped-for series of moves away from a monolithic
common.js that is loaded for every test and towards a more modular
approach. (In the end, common.js will hopefully contain checks for
variables leaking into the global space and perhaps some of the more
ubiquitous functions like common.mustCall().)

Move the WPT testing code to its own module.

PR-URL: #12736
Backport-PR-URL: #13775
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
@MylesBorins MylesBorins referenced this pull request Jul 18, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
10 participants
You can’t perform that action at this time.