Skip to content

Use current working directory if no package.json #1044

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

Merged
merged 10 commits into from
Oct 30, 2016
Merged

Use current working directory if no package.json #1044

merged 10 commits into from
Oct 30, 2016

Conversation

sotojuan
Copy link
Contributor

Fixes #1041.

Currently we give a nasty error when AVA is ran in a directory with no package.json. If none is found, we fallback to the current working directory.

@sotojuan
Copy link
Contributor Author

This might interfere with #32, not sure.

@sotojuan sotojuan closed this Sep 21, 2016
@sotojuan sotojuan reopened this Sep 21, 2016

try {
pkgDir = path.dirname(pkgConf.filepath(conf));
} catch (err) {
Copy link
Member

Choose a reason for hiding this comment

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

Check if pkgConf.filepath(conf) returns null instead. If it does, use process.cwd(), else call path.dirname() with the return value.

@sotojuan
Copy link
Contributor Author

Done!

pkgDir = process.cwd();
} else {
pkgDir = path.dirname(filepath);
}
Copy link
Member

Choose a reason for hiding this comment

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

Would be better as a simple ternary:

var pkgDir = filepath === null ? process.cwd() : path.dirname(filepath);

@sotojuan
Copy link
Contributor Author

Right, of course :p Fixed.

@jamestalmage
Copy link
Contributor

Any way to test this?

I'm thinking something like:

const dir = uniqueTempDir({create: true});
const testFilePath = path.join(dir, 'test.js');
const cliPath = require.resolve('../../cli.js');

fs.writeFileSync(testFilePath, `
  import test from 'ava';

  test(t => {
    t.pass();
  });
`);

execa(process.execPath, [cliPath], {cwd: dir}).then(endTheTest);

@sindresorhus
Copy link
Member

@sotojuan Could you add a test?

@sotojuan
Copy link
Contributor Author

sotojuan commented Oct 6, 2016

Yep, sorry I got busy these past two weeks! Test + improvements coming up soon.

@sotojuan
Copy link
Contributor Author

Any idea of why it's failing? Works locally (and test critique please).

@vadimdemedes
Copy link
Contributor

Also passed locally on clean node_modules without a problem.

var dir = uniqueTempDir({create: true});
var testFilePath = path.join(dir, 'test.js');
var cliPath = require.resolve('../cli.js');
var avaPath = require.resolve('../index.js');
Copy link
Member

Choose a reason for hiding this comment

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

var avaPath = require.resolve('../');

@@ -368,3 +371,17 @@ test('prefers local version of ava', function (t) {
t.ok(debugSpy.calledWith('Using local install of AVA'));
t.end();
});

test('use current working directory if `package.json` is not found', function (t) {
var dir = uniqueTempDir({create: true});
Copy link
Member

Choose a reason for hiding this comment

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

dir => cwd

execa(process.execPath, [cliPath], {cwd: dir}).then(() => {
t.pass();
t.end();
});
Copy link
Member

Choose a reason for hiding this comment

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

It needs to handle a possibly rejected promise too.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think you should just return the promise. You don't test the output of the command, so no need for manual then/catch. Return a promise and test will fail, if the promise rejects.

Copy link
Member

@sindresorhus sindresorhus Oct 10, 2016

Choose a reason for hiding this comment

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

👍 Ah true. Forgot node-tap got support for promises.

@sotojuan
Copy link
Contributor Author

Still breaking, but updated.

@novemberborn
Copy link
Member

@sotojuan I've found why the test was failing, there's another bug where we assume we can always find a cache dir. That's not true when running outside of a directory with a package.json file!

Pushed a fix for that. Also merged master into this branch which is a bit unconventional but I didn't want to force-push ;-) Squash & merge will take care of that for us.

Juan Soto added 2 commits October 14, 2016 14:57
Update test

Document more async pitfalls (#1045)

* Use tabs in pitfalls code blocks

* Spell out async in pitfals doc

* Promote async/await in pitfalls doc

* Provide example of using pify

* Suggest pify for catching exceptions

* Suggest promisifying callback-functions for better exception catching

Do not say that `t` is the only argument of a test

bump all the `babel` related modules to latest

Some older versions that are still matched by semver were buggy and some users were somehow getting those versions. Maybe from a stale npm cache.

Fixes #1058

bump `tap` and `nyc`

highlights - Automatic migration from other test runners

Thanks @skovhus! :)

Replace `jsdom` with `browser-env` in browser recipe (#1054)

Clean up API (#1061)

Switch to `lodash.isEqual` for deep quality check (#1063)

Add link to first contribution blog post

Reduce transpilation on Node.js >=4 (#1068)

Better startup performance and improved stack traces.

Remove --require CLI option (#1070)

Fixes #1069.

Fix link to configuration in readme (#1073)

make XO happy

Locking the version as 0.17.0 requires Node.js 4

Check whether a cache dir was found before attempting to use it

AVA will never find a cache dir if it is run in a directory without a package.json file.
@sotojuan
Copy link
Contributor Author

Phew, had to fix some git stuff and merge latest master. If the tests pass I can squash.

@sotojuan
Copy link
Contributor Author

Boo AppVeyor.

@sindresorhus sindresorhus force-pushed the master branch 2 times, most recently from 4bc021e to 476c653 Compare October 18, 2016 03:23
@sindresorhus
Copy link
Member

AppVeyor error:

Error: Cannot find module 'C:projectsavaindex.js

My first thought; Where are the path separators?

var cliPath = require.resolve('../cli.js');
var avaPath = require.resolve('../');

fs.writeFileSync(testFilePath, 'import test from \'' + avaPath + '\';\ntest(t => { t.pass(); });');
Copy link
Member

Choose a reason for hiding this comment

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

Changing to this probably fixes the AppVeyor error:

'import test from ' + JSON.stringify(avaPath) + ';\ntest(t => { t.pass(); });'

I reckon on Windows the path contains backslashes which are interpreted as escape characters.

@sotojuan
Copy link
Contributor Author

All right I'm pretty sure AppVeyor will like these changes.

@sotojuan
Copy link
Contributor Author

🐔

@novemberborn novemberborn dismissed sindresorhus’s stale review October 30, 2016 14:54

Changes have been addressed

@novemberborn novemberborn merged commit 59254b7 into avajs:master Oct 30, 2016
@sotojuan
Copy link
Contributor Author

Thanks!

@sotojuan sotojuan deleted the fallback-pkgdir branch October 30, 2016 15:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants