Change the current working directory of tests to be the same directory as package.json #1074

alathon · 2016-10-13T07:45:05Z

Description

Changes fork to spawn the process based on the (existing) value of options.resolveTestsFrom, which is the package directory by default. Fixes, amongst other things, problems with babel-root-import, which does not work with the current behavior (as it expects process.cwd() to be the package.json folder).

Related to #32 and several other issues that stem from the same problem.

I'm not 100% sure that I'm meant to be piggybacking options.resolveTestsFrom for this purpose - but it seems like the appropriate option. There were no tests of resolveTestsFrom at all, so I'm not 100% sure what the option is meant to be doing, if not this.

sindresorhus · 2016-10-13T07:47:02Z

You also need to update this paragraph in the readme:

Test files are run from their current directory, so process.cwd() is always the same as __dirname. You can just use relative paths instead of doing path.join(__dirname, 'relative/path').

alathon · 2016-10-13T08:17:57Z

@sindresorhus I've updated the readme to reflect the change, or rather how I'd expect it to work. However, now I'm slightly in doubt as to whether resolveTestsFrom should be implemented in cli.js as it is.

I'm confused by why api.js:140 (https://github.com/alathon/ava/blob/use-resolveTestsFrom-in-fork/cli.js#L140) is implemented like that. Can someone elaborate on why resolveTestsFrom gets set to process.cwd() if there is non-flag input, and why resolveTestsFrom wasn't made a CLI flag to begin with? Its a bit hard for me to reason about since there are zero tests of resolveTestsFrom.

sindresorhus · 2016-10-13T08:21:24Z

readme.md

@@ -247,7 +247,7 @@ If you're unable to use promises or observables, you may enable "callback mode"

 You must define all tests synchronously. They can't be defined inside `setTimeout`, `setImmediate`, etc.

-Test files are run from their current directory, so [`process.cwd()`](https://nodejs.org/api/process.html#process_process_cwd) is always the same as [`__dirname`](https://nodejs.org/api/globals.html#globals_dirname). You can just use relative paths instead of doing `path.join(__dirname, 'relative/path')`.
+The process that runs a test file has a [`process.cwd()`](https://nodejs.org/api/process.html#process_process_cwd) that is set to the directory of the package.json file AVA finds. In most projects this will be the root folder of the project. To override this behavior and run tests from a *different* folder, you can set the `resolveTestsFrom` option to a relative path, which will be used instead of the package file directory.


Drop the following: To override this behavior and run tests from a *different* folder, you can set theresolveTestsFromoption to a relative path, which will be used instead of the package file directory.

It doesn't make sense here as it's for the API, not the consumer. I don't think we want it documented at all as the API is not stable.

Good point. Removed that part again. Is it relevant to discuss my question above about resolveTestsFrom in a separate issue, or is Gitter more appropriate for that?

alathon · 2016-10-13T09:30:22Z

@novemberborn mentions that resolveTestsFrom is used to resolve command-line paths to files. That means I'm misusing it here, and ironically you'd end up with the same problematic behavior, if you ran ava from a sub-folder.

I've gone ahead and added a package option called pkgDir instead, which does the same thing, and changed the tests accordingly.

novemberborn

Looking good, @alathon. Just a comment on the tests really, and I think we can simplify the README comment a bit more too.

novemberborn · 2016-10-14T14:07:06Z

test/api.js

@@ -307,12 +307,23 @@ function generateTests(prefix, apiCreator) {
 			});
 	});

-	test(prefix + 'change process.cwd() to a test\'s directory', function (t) {
+	test(prefix + 'run from package.json folder by default', function (t) {


This test doesn't do what you think it does.

The apiCreator executor doesn't actually set the pkgDir option when it creates the api. This means that the child process is forked with an undefined cwd option, so it ends up using process.cwd(). Because we run tests from the project root that happens to be the expected directory, but that's just luck.

Instead let's find the correct pkgDir value in the test itself, and provide it when creating the API on this line and this one.

The next test is good (since it tests a non-default value). Just make sure not to accidentally break it with the aforementioned changes.

Ahh yeah, I see what you're saying. The fact that its even possible to create an Api() leading to an undefined pkgDir being given to fork is probably not a good idea. What about doing the following:

Set pkgDir in the same way we do now, but in api.js instead, around https://github.com/alathon/ava/blob/2a65c1295588ebdf36dcc833484c34f3ed956add/api.js#L55

Remove the option from cli.js (making it a flag you can set through the CLI is a decent idea, but probably a separate PR)

This would mean the test would work as expected, and test what it says - that if you just create an Api(), then it'll run from the package.json folder by default.

I changed things to reflect what I suggest here. The commit history is a bit of a mess now though, due to the rebase from master happening before a pull/push. My bad ^^

novemberborn · 2016-10-14T14:13:31Z

readme.md

@@ -247,7 +247,7 @@ If you're unable to use promises or observables, you may enable "callback mode"

 You must define all tests synchronously. They can't be defined inside `setTimeout`, `setImmediate`, etc.

-Test files are run from their current directory, so [`process.cwd()`](https://nodejs.org/api/process.html#process_process_cwd) is always the same as [`__dirname`](https://nodejs.org/api/globals.html#globals_dirname). You can just use relative paths instead of doing `path.join(__dirname, 'relative/path')`.
+The process that runs a test file has a [`process.cwd()`](https://nodejs.org/api/process.html#process_process_cwd) that is set to the directory of the package.json file AVA finds. In most projects this will be the root folder of the project.


How about simply:

AVA tries to run test files with their current working directory set to the directory that contains your package.json file.

(Please mark the package.json bit as code.)

Thats a lot more concise, nice :)

novemberborn · 2016-10-14T14:15:20Z

If you rebase against master you should be able to make CI pass.

sindresorhus · 2016-10-14T19:45:04Z

cli.js

@@ -139,6 +139,7 @@ var api = new Api({
 	babelConfig: conf.babel,
 	resolveTestsFrom: cli.input.length === 0 ? pkgDir : process.cwd(),
 	timeout: cli.flags.timeout,
+	pkgDir,


You can't use object literal shorthand. We still support older Node.js versions.

novemberborn · 2016-10-15T15:25:08Z

Ahh yeah, I see what you're saying. The fact that its even possible to create an Api() leading to an undefined pkgDir being given to fork is probably not a good idea.

Yea that's kinda what #611 is about: making the interfaces more explicit.

What about doing the following:

Set pkgDir in the same way we do now, but in api.js instead, around https://github.com/alathon/ava/blob/2a65c1295588ebdf36dcc833484c34f3ed956add/api.js#L55

I'd rather we keep passing it in, test issues aside. For instance #1041 relates to this PR as well, and we should just solve that in cli.js.

Could you go back to the previous implementation and fix the test instead? I'm actually OK with leaving the option implicit for the other places where tests create API instances.

I changed things to reflect what I suggest here. The commit history is a bit of a mess now though, due to the rebase from master happening before a pull/push. My bad ^^

No worries, we'll just squash and merge it.

alathon · 2016-10-15T18:04:30Z

@novemberborn I've reverted the PR to the previous version where pkgDir is specified in cli.js, and fixed the tests.

alathon · 2016-10-15T21:38:41Z

cli.js

-	'  --source, -S       Pattern to match source files so tests can be re-run (Can be repeated)',
-	'  --timeout, -T      Set global timeout',
-	'  --concurrency, -c  Maximum number of test files running at the same time (EXPERIMENTAL)',
+	'  --init							Add AVA to your project',


Eh, my editor seems to have done something odd with the tabs here.. Will fix

novemberborn

Almost there, nice work @alathon.

novemberborn · 2016-10-16T12:08:54Z

test/api.js

@@ -18,6 +21,7 @@ test('must be called with new', function (t) {
 generateTests('Without Pool: ', function (options) {
 	options = options || {};
 	options.powerAssert = true;
+	options.pkgDir = options.pkgDir || path.dirname(pkgConf.filepath(conf));


Could you recompute this on line 11? It's run every time a test needs to create an API which seems unnecessary.

novemberborn · 2016-10-16T17:00:56Z

You might be about to push the commit, in which case, apologies, but don't forget to fix the whitespace in the README 😉

Update description of where tests are run from.

Drop mention of overriding `resolveTestsFrom`

More concise description of where tests run from.

alathon · 2016-10-17T09:18:56Z

The Travis CI failures seem to be related to upgrading xo, am I right in this @sindresorhus ?

sindresorhus · 2016-10-17T09:44:15Z

@alathon Fixed in 6165a23.

novemberborn · 2016-10-17T11:01:52Z

cli.js

+	'  --watch, -w				Re-run tests when tests and source files change',
+	'  --source, -S			Pattern to match source files so tests can be re-run (Can be repeated)',
+	'  --timeout, -T			Set global timeout',
+	'  --concurrency, -c			Maximum number of test files running at the same time (EXPERIMENTAL)',


@alathon still seeing a diff here.

Yes, there is a diff. But it lines up correctly in the terminal, when I execute it now. I'm not sure how to un-diff just that particular part of the file, although I know its possible through git.

Well it renders the same so I'm not too bothered I suppose. @sindresorhus?

They're different as you changed from tabs to spaces. They should remain spaces.

I'll fix it on merge, but try not to do unrelated changes when doing a PR ;)

You could have uncommitted that part btw. Would be easiest to use a GUI git app for that. I use GitUp.app.

…m-in-fork

alathon · 2016-10-17T11:39:51Z

Branch updated with changes from upstream master now (to fix xo errors and make it easier to squash/rebase).

sindresorhus · 2016-10-17T17:45:04Z

Thanks @alathon :)

sindresorhus · 2016-10-17T17:45:13Z

@novemberborn This fixes #32, right?

…y as package.json (#1074) #32

alathon changed the title ~~Use resolve tests from in fork~~ Use CLI option resolveTestsFrom in lib/fork Oct 13, 2016

alathon changed the title ~~Use CLI option resolveTestsFrom in lib/fork~~ Use resolveTestsFrom in lib/fork Oct 13, 2016

sindresorhus reviewed Oct 13, 2016

View reviewed changes

alathon changed the title ~~Use resolveTestsFrom in lib/fork~~ Use pkgDir in lib/fork Oct 13, 2016

novemberborn requested changes Oct 14, 2016

View reviewed changes

sindresorhus reviewed Oct 14, 2016

View reviewed changes

alathon commented Oct 15, 2016

View reviewed changes

novemberborn requested changes Oct 16, 2016

View reviewed changes

alathon added 13 commits October 17, 2016 09:34

Fork child processes based on Api.options.resolveTestsFrom

c97a404

Modify process.cwd test to account for resolveTestsFrom

844821f

Add test that verifies CWD as pkgDir by default

033ff7b

Change to tabs and remove commented out line

d4ca886

Update readme.md

efd095b

Update description of where tests are run from.

Update readme.md

48831d1

Drop mention of overriding `resolveTestsFrom`

Added pkgDir option instead of using resolveTestsFrom

aa80383

Move pkgDir to api.js as a default option

20a7ca8

Update readme.md

0260bff

More concise description of where tests run from.

Revert to specifying pkgDir in cli.js

571f788

Remove accidentally pushed console.log statements

2ca98ee

Precompute pkgDir

88a1706

Re-align CLI --help

ab36f0d

novemberborn requested changes Oct 17, 2016

View reviewed changes

novemberborn approved these changes Oct 17, 2016

View reviewed changes

Merge remote-tracking branch 'origin/master' into use-resolveTestsFro…

f1d21ba

…m-in-fork

alathon force-pushed the use-resolveTestsFrom-in-fork branch from 0711e93 to f1d21ba Compare October 17, 2016 11:38

sindresorhus changed the title ~~Use pkgDir in lib/fork~~ Change the current working directory of tests to be the same directory as package.json Oct 17, 2016

sindresorhus merged commit edc41a1 into avajs:master Oct 17, 2016

sindresorhus pushed a commit that referenced this pull request Oct 17, 2016

Change the current working directory of tests to be the same director…

7634ace

…y as package.json (#1074) #32

sindresorhus pushed a commit that referenced this pull request Oct 17, 2016

Change the current working directory of tests to be the same director…

4bc021e

…y as package.json (#1074) #32

sindresorhus pushed a commit that referenced this pull request Oct 18, 2016

Change the current working directory of tests to be the same director…

476c653

…y as package.json (#1074) #32

jgkim added a commit to tipplrio/eslint-config-tipplr that referenced this pull request Dec 12, 2016

Fix the broken test due to avajs/ava#1074

729edc2

jonathantneal mentioned this pull request May 11, 2017

Use PostCSS 6 postcss/postcss-import#283

Merged

CodeLenny mentioned this pull request Jul 23, 2017

process.cwd() conflicts in cleanup rrdelaney/ava-rethinkdb#4

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change the current working directory of tests to be the same directory as package.json #1074

Change the current working directory of tests to be the same directory as package.json #1074

alathon commented Oct 13, 2016

sindresorhus commented Oct 13, 2016

alathon commented Oct 13, 2016 •

edited

Loading

sindresorhus Oct 13, 2016

alathon Oct 13, 2016

alathon commented Oct 13, 2016

novemberborn left a comment

novemberborn Oct 14, 2016

alathon Oct 14, 2016

alathon Oct 14, 2016

novemberborn Oct 14, 2016

alathon Oct 14, 2016

novemberborn commented Oct 14, 2016

sindresorhus Oct 14, 2016

novemberborn commented Oct 15, 2016

alathon commented Oct 15, 2016 •

edited

Loading

alathon Oct 15, 2016

novemberborn left a comment

novemberborn Oct 16, 2016

novemberborn commented Oct 16, 2016

alathon commented Oct 17, 2016

sindresorhus commented Oct 17, 2016

novemberborn Oct 17, 2016

alathon Oct 17, 2016

novemberborn Oct 17, 2016

sindresorhus Oct 17, 2016

sindresorhus Oct 17, 2016

alathon commented Oct 17, 2016

sindresorhus commented Oct 17, 2016

sindresorhus commented Oct 17, 2016

Change the current working directory of tests to be the same directory as package.json #1074

Change the current working directory of tests to be the same directory as package.json #1074

Conversation

alathon commented Oct 13, 2016

Description

sindresorhus commented Oct 13, 2016

alathon commented Oct 13, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alathon commented Oct 13, 2016

novemberborn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

novemberborn commented Oct 14, 2016

Choose a reason for hiding this comment

novemberborn commented Oct 15, 2016

alathon commented Oct 15, 2016 • edited Loading

Choose a reason for hiding this comment

novemberborn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

novemberborn commented Oct 16, 2016

alathon commented Oct 17, 2016

sindresorhus commented Oct 17, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alathon commented Oct 17, 2016

sindresorhus commented Oct 17, 2016

sindresorhus commented Oct 17, 2016

alathon commented Oct 13, 2016 •

edited

Loading

alathon commented Oct 15, 2016 •

edited

Loading