test,doc: document where common modules go #16089
Conversation
|
I feel like this probably isn't correct - wouldn't we want tests-specific helper modules all imported at the top? Also, maybe the example can actually use fixtures somehow? It's probably just going to be confusing having it there with no purpose. :/ |
See:
I originally had it with the main common line, but AIUI the reason that is at the top is because it's mandatory due to global checking etc. Requiring
See:
We'd probably want to change the whole test, and frankly this is there as an example test, it doesn't need to use everything it contains, it should contain all the stuff you might want to use. So I considered it but decided not to bother. Thoughts? |
Maybe, in this case, we can add // ...at the end of the example? |
|
+1 for a lint rule for this! |
gibfahn
force-pushed
the
guide-fixtures
branch
from
ad06435
to
5a32787
Compare
doc/guides/writing-tests.md
Outdated
| server.close(); // 19 | ||
| })); // 20 | ||
| }); // 21 | ||
| const fixtures = require('../common/fixtures'); // 8 |
There was a problem hiding this comment.
can we at least move this to either above or below the core module imports?
There was a problem hiding this comment.
Maybe we should just put it under the require('common') line then, so all the common modules are in the same place.
gibfahn
force-pushed
the
guide-fixtures
branch
from
5a32787
to
e3c85ed
Compare
Keep the `require('../common')` separate from other common modules, as
it's the only line that must be there.
gibfahn
force-pushed
the
guide-fixtures
branch
from
e3c85ed
to
b6edc63
Compare
|
@Fishrock123 updated, PTAL. |
Keep the `require('../common')` separate from other common modules, as
it's the only line that must be there.
PR-URL: #16089
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: James M Snell <[email protected]>
|
Landed in 17db46c |
Keep the `require('../common')` separate from other common modules, as
it's the only line that must be there.
PR-URL: #16089
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Keep the `require('../common')` separate from other common modules, as
it's the only line that must be there.
PR-URL: #16089
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Keep the `require('../common')` separate from other common modules, as
it's the only line that must be there.
PR-URL: #16089
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Keep the `require('../common')` separate from other common modules, as
it's the only line that must be there.
PR-URL: #16089
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Keep the
require('../common')separate from other common modules, asit's the only line that must be there.
I noticed that in the code-and-learn PRs, people are putting the
require('../common/fixtures')require in with the normal ones. I think this is the right place to put it, but I'm open to suggestions.Also the
fixturesmodule isn't actually used in this example test, I could probably find another test that uses it, but will bloat the diff for questionable gain (making the lint pass on the sample test).cc/ @nodejs/testing
Checklist
Affected core subsystem(s)
test