avajs / ava

Node.js test runner that lets you develop with confidence 🚀
MIT License
20.74k stars 1.41k forks source link

Should `process.cwd()` be changed to the directory of the currently running test? #32

Closed sindresorhus closed 8 years ago

sindresorhus commented 9 years ago

Currently the cwd is the directory you execute ava in, but ava can be executed from any upper level in the project and will search down for test files, so the cwd is pretty arbitrary and useless. I want to make the cwd actually useful.

There are two options:

  1. Change cwd to root of the project. (This is what gulp/grunt does)
  2. Change cwd to the directory of the currently running test file. (This is what I personally prefer)

The reason I prefer the latter is that we run the test files hence IMHO the cwd is where we run it. It's like we would do: cd tests && node test.js.

Currently you have to do this to be able to load a read a test relative file:

var path = require('path');
var fs = require('fs');
fs.readFileSync(path.join(__dirname, 'foo.json'));

If we changed process.cwd() to the currently running test directory we could instead just do:

var fs = require('fs');
fs.readFileSync('foo.json');

Thoughts? Anything I'm missing?

mdibaiee commented 9 years ago

I think it's a good idea but I'm not sure if it has any drawbacks as I lack experience in this kind of thing.

Qix- commented 9 years ago

It sounds good, but it might make migrating from Mocha a bit of a pain. New projects will benefit from it; ones coming from Mocha won't.

sindresorhus commented 9 years ago

@Qix- Why? It will make no difference if you've already hard-coded your paths with __dirname.

Qix- commented 9 years ago

True. Those that haven't should be anyway.

vadimdemedes commented 9 years ago

:+1: Also tired of join(__dirname, 'fixtures', 'shit')

kevva commented 9 years ago

Yeah, I like option two too because that's really what is expected. Using the project root would confuse me.

arthurvr commented 9 years ago

Yeah, I like option two too because that's really what is expected. Using the project root would confuse me.

Me too. :+1:

stevemao commented 9 years ago

I think this is more intuitive for people who haven't used mocha. But would this confuse people who just learned require(./) and fs.readFile(./) as the path are relative to different base?

sindresorhus commented 9 years ago

But would this confuse people who just learned require(./) and fs.readFile(./) as the path are relative to different base?

That's not correct. If you start the node script from the file location they're the same. The point is that running a test is kinda like doing node test.js from the test file location.

stevemao commented 9 years ago

That's not correct. If you start the node script from the file location they're the same. The point is that running a test is kinda like doing node test.js from the test file location.

True, very intuitive because this is what I thought when I first used mocha :)

jamestalmage commented 8 years ago

I have been pretty consistently been annoyed by this "feature". Yes require(../some/path) is relative, but if I am using require - it's at the top of my test, and easy to find. If I am using cwd, well, that could be anywhere. And it's not always easy to search for:

// relies on cwd - but it isn't apparent how use my IDE's search tool to find it.
execa('node foo.js');

Moving a test that relies on child_process.exec or execa wreaks havoc since we implemented this.

Conversely paths relative to the module file are pretty easy to locate. Look for __dirname and require (./ if things get really desperate). If I am moving a test file around (and not moving some resource with it) the search and replace is pretty easy.

I would have preferred that cwd was always set to to the same directory as package.json (found via pkgUp). That has the advantage of keeping cwd consistent, even when you execute ava from a subdir, and it allows you to treat cwd as a base for creating "intra-project-absolute paths".

sindresorhus commented 8 years ago

@jamestalmage I agree. In hindsight, it was a bad idea with good intentions. Too bad it didn't show its downsides until much later. I've personally been bitten by this on multiple occasions too and seen some problems with it in the ecosystem. I think we should reconsider. I've never seen anyone other than me use it anyways (I've looked at a gadzillion open source AVA test files).

Some known problems this feature caused:

@vdemedes @novemberborn @sotojuan @jedrichards @istarkov @alexbooker @MarkHerhold @KidkArolis

novemberborn commented 8 years ago

Relative imports are more cumbersome than relative file paths. I like setting cwd to the package.json directory, though that might be a little tricky for monorepo's.

jamestalmage commented 8 years ago

though that might be a little tricky for monorepo's

Not sure why it would be. Usually there is a package.json for each package in the monorepo - so it would just be relative to that. If there's not (some people auto-generate at deploy time) then you would be relative to the root package.json.

sindresorhus commented 8 years ago

Ok, we're going to do this.

sindresorhus commented 8 years ago

We might need some codemods for this change: https://github.com/jamestalmage/ava-codemods/issues/19

novemberborn commented 8 years ago

@a-s-o let's discuss your suggestion here, not in https://github.com/avajs/ava-codemods/issues/19#issuecomment-220839269 :)

Hi, may I also request not using the nearest package.json as the CWD (i.e looking package.json upwards from the test file). This may cause issues in multi-package repos where the test runner is in the root but there are multiple packages with their own package.json in the project (example the babel repo).

I would like to suggest sticking with the directory where ava command is executed as that is the most obvious.

Currently we look at the package.json to load AVA config. We start the search for this file in the current working directory, so if you're executing AVA in the monorepo root, it would use the root's package.json. I imagine we'll do the same when determining the CWD for the test files.

Is this what you mean by "sticking with the directory where ava command is executed"?

a-s-o commented 8 years ago

Is this what you mean by "sticking with the directory where ava command is executed"?

I think you have it right, but to clarify, let's say I have the following directory structure

project_root
  +-- api
  |     +-- tests
  |     |     |-- api_test_1.js
  |     |     |-- api_test_2.js
  |     |-- api.js
  |     |-- package.json
  |
  |-- package.json

So, my suggestions is as follows:

~/project_root> ava **/tests/*.js         <-- process.cwd is ~/project_root
~/project_root/api> ava **/tests/*.js     <-- process.cwd is ~/project_root/api

Basically, cwd should be the working directory where the ava command was launched. If it gets launched from a npm script, then the cwd is wherever that npm script is located.

Hope that helps

novemberborn commented 8 years ago

So, my suggestions is as follows:

That's how it would work, but due to the presence of the package.json.

Basically, cwd should be the working directory where the ava command was launched.

That becomes a problem when you launch ava from project_root/api/tests. We try to pick a consistent directory no matter where you start the tests from. Originally that was the directory the test file was in, but now it'll be the directory the package.json is in.

This behavior breaks down a little for monorepos with multiple package.json files, but to be fair so does npm test which doesn't look past the first package.json file.

jamestalmage commented 8 years ago

We could add a cwd config option to package.json

a-s-o commented 8 years ago

You are right, that is the default npm behaviour. Okay then that is a good enough convention to follow. I can live with that. Thanks for the info. On May 22, 2016 1:57 PM, "James Talmage" notifications@github.com wrote:

We could add a cwd config option to package.json

— You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub https://github.com/avajs/ava/issues/32#issuecomment-220846264

jamestalmage commented 8 years ago

Another bug caused by our current behavior: https://github.com/dtinth/babel-plugin-__coverage__/issues/39

We should revert this for the next release.

KidkArolis commented 8 years ago

What's the status of this, changing cwd to the test file location is really annoying.

sindresorhus commented 8 years ago

It's blocked on having a migrate script: https://github.com/avajs/ava/issues/32#issuecomment-218998130 The change itself is simple.

shinzui commented 8 years ago

@sindresorhus I don't think the codemod should block this issue. I just started using ava and I wasted a lot of time figuring out why hapi was not loading my models correctly in the tests. I initially blamed hapi-shelf (plugin to load bookshelf models), but after tracing the code I discovered this issue and easily fixed my test to work around it.

sindresorhus commented 8 years ago

Yeah, let's just do the change.

It's hard to do a codemod for this correctly anyway, and I don't think it's important enough to force us to continue having surprising behavior for new users.

@avajs/core ?

novemberborn commented 8 years ago

👍

wmertens commented 8 years ago

So is anybody working on this? I'm not volunteering, just doing a "ping" 😀

sindresorhus commented 8 years ago

@wmertens https://twitter.com/slicknet/status/782274190451671040

novemberborn commented 8 years ago

Fixed by 476c65396155a7d16fbdd55c65863ecaabd873de.