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

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

Merged
merged 14 commits into from
Oct 17, 2016
Merged
25 changes: 13 additions & 12 deletions cli.js
Original file line number Diff line number Diff line change
Expand Up @@ -57,18 +57,18 @@ var cli = meow([
' ava [<file|directory|glob> ...]',
'',
'Options',
' --init Add AVA to your project',
' --fail-fast Stop after first test failure',
' --serial, -s Run tests serially',
' --tap, -t Generate TAP output',
' --verbose, -v Enable verbose output',
' --no-cache Disable the transpiler cache',
' --no-power-assert Disable Power Assert',
' --match, -m Only run tests with matching title (Can be repeated)',
' --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)',
' --init Add AVA to your project',
' --fail-fast Stop after first test failure',
' --serial, -s Run tests serially',
' --tap, -t Generate TAP output',
' --verbose, -v Enable verbose output',
' --no-cache Disable the transpiler cache',
' --no-power-assert Disable Power Assert',
' --match, -m Only run tests with matching title (Can be repeated)',
' --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)',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alathon still seeing a diff here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

'',
'Examples',
' ava',
Expand Down Expand Up @@ -138,6 +138,7 @@ var api = new Api({
match: arrify(cli.flags.match),
babelConfig: conf.babel,
resolveTestsFrom: cli.input.length === 0 ? pkgDir : process.cwd(),
pkgDir: pkgDir,
timeout: cli.flags.timeout,
concurrency: cli.flags.concurrency ? parseInt(cli.flags.concurrency, 10) : 0
});
Expand Down
2 changes: 1 addition & 1 deletion lib/fork.js
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ module.exports = function (file, opts, execArgv) {
}, opts);

var ps = childProcess.fork(path.join(__dirname, 'test-worker.js'), [JSON.stringify(opts)], {
cwd: path.dirname(file),
cwd: opts.pkgDir,
silent: true,
env: env,
execArgv: execArgv || process.execArgv
Expand Down
2 changes: 1 addition & 1 deletion readme.md
Original file line number Diff line number Diff line change
Expand Up @@ -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')`.
AVA tries to run test files with their current working directory set to the directory that contains your `package.json` file.

### Creating tests

Expand Down
23 changes: 20 additions & 3 deletions test/api.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,13 @@ var fs = require('fs');
var figures = require('figures');
var rimraf = require('rimraf');
var test = require('tap').test;
var pkgConf = require('pkg-conf');
var Api = require('../api');
var testCapitalizerPlugin = require('./fixture/babel-plugin-test-capitalizer');

var conf = pkgConf.sync('ava');
var pkgDir = path.dirname(pkgConf.filepath(conf));

test('must be called with new', function (t) {
t.throws(function () {
var api = Api;
Expand All @@ -18,6 +22,7 @@ test('must be called with new', function (t) {
generateTests('Without Pool: ', function (options) {
options = options || {};
options.powerAssert = true;
options.pkgDir = options.pkgDir || pkgDir;
return new Api(options);
});

Expand Down Expand Up @@ -63,6 +68,7 @@ generateTests('With Pool: ', function (options) {
options = options || {};
options.concurrency = 2;
options.powerAssert = true;
options.pkgDir = options.pkgDir || pkgDir;
return new Api(options);
});

Expand Down Expand Up @@ -307,12 +313,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) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. 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
  2. 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ^^

t.plan(1);

var api = apiCreator();

return api.run([path.join(__dirname, 'fixture/process-cwd.js')])
return api.run([path.join(__dirname, 'fixture/process-cwd-default.js')])
.then(function (result) {
t.is(result.passCount, 1);
});
});

test(prefix + 'change process.cwd() to a test\'s directory with pkgDir', function (t) {
t.plan(1);

var fullPath = path.join(__dirname, 'fixture/process-cwd-pkgDir.js');

var api = apiCreator({pkgDir: path.dirname(fullPath)});
return api.run([fullPath])
.then(function (result) {
t.is(result.passCount, 1);
});
Expand Down
9 changes: 9 additions & 0 deletions test/fixture/process-cwd-default.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
import test from '../../';
import pkgConf from 'pkg-conf'
import path from 'path'

test(t => {
var conf = pkgConf.sync('ava')
var pkgDir = path.dirname(pkgConf.filepath(conf))
t.is(process.cwd(), pkgDir)
});
File renamed without changes.