Deeply recurse directories

[ariporad]

This changes the current behavior when a user specifies a directory in a glob pattern, i.e.:

Consider:

$ ava tests

Currently, this would only include all .js files in the tests directory, but not subdirectories.

With this change, all .js files in the tests directory and all sub directories (infinitely deep) will be included.

Fixes #249.

[jamestalmage]

I don't think we should make this particular change. It changes the default behavior of simply running ava. The default behavior should remain the same. If they want to use this feature they should have to run ava test.

[ariporad]

@jamestalmage: yes, it would be a breaking change. But otherwise we have Inconsistent and confusing behavior.

[jamestalmage]

I think what you are doing here is inconsistent and confusing.

The default is currently the same as running:

ava test.js test-*.js test/*.js

If we don't make this change that stays consistent. To engage the "recursive directory behavior" they should need explicitly provide a directory name they want recursed. There is less chance of an error that way. I find it far more confusing that there are .js globs for the base-dir, but we will recurse the test folder indefinitely.

[sindresorhus]

@jamestalmage I know it's a breaking change, but it's what people except (#249). Ideally, no one should have to specify anything and it should just work. By recursing test by default, but ignoring the common helper and fixture dir, we can have it just work with convention. What kind of error are you afraid of?

I find it far more confusing that there are .js globs for the base-dir, but we will recurse the test folder indefinitely.

Even though we explicitly specify the default glob patterns?

[ariporad]

@sindresorhus: I kind of see where @jamestalmage is coming from, the some things recursive some not is a little confusing.

I'd like to purpose that we do something like:

**/test.js **/test-*.js test/**/*.js, **/*.spec.js **/*.test.js

[ariporad]

@sindresorhus: why isn't it safe to recurse the entire directory? We already ensure that node_modules is ignored.

And I'm asking for it. I always structure my modules with the test next to the file. (I have a really cool system, I can explain more if you like).

[sindresorhus]

why isn't it safe to recurse the entire directory?

I just answered that: "This would match https://github.com/sindresorhus/ava/blob/master/lib/test.js" Which is not a test file.

I have a really cool system, I can explain more if you like

Please do :)

[ariporad]

@sindresorhus: Well, I may have oversold it a little, but here's my setup:

I start with a file structure like this:

my-app
├── src/                     # Code
|   ├── foo.js               # Some fascinating module
|   ├── foo.test.unit.js     # The unit (doesn't exit the process, <150ms/test) tests for `foo.js`
|   ├── bar.js               # Another fascinating module
|   ├── bar.test.int.js      # The focused integration tests for `bar.js`
|   └── ...                  # Other stuff too
├── test/                    # Tests that don't belong to a single source file, utils, mocks, fixtures, etc.
|   ├── utils/               # Test utilities
|   |   ├── findPort.js      # Find an available port.
|   |   ├── portAvail.js     # Test if a port is available.
|   |   └── ...              # Other stuff too
|   ├── fixture/             # Test fixtures
|   |   ├── some-test        # Find an available port.
|   |   └── ...              # Other stuff too
|   ├── mocks/               # Some mocks 
|   |   ├── SomeClass.js     # A mock for `SomeClass`
|   |   └── ...              # Other stuff too
|   ├── webapp.test.e2e.js   # End2End tests for the web app
|   ├── mobile.test.e2e.js   # End2End tests for the mobile app
|   ├── api.test.e2e.js      # End2End tests for the API
|   ├── setup.js             # Register some global helper functions, and bluebird.
|   └── ...                  # Other stuff too
└── ...                      # Other stuff too

Then you use app-module-path for my-app, and then your tests look like this:

import test from 'ava';
import rewire from 'rewire';
import findPort from 'test/utils/findPort';
import portAvail from 'test/utils/portAvail';
import FakeSomeClass from 'test/mock/SomeClass';

test.beforeEach(async t => {
    t.context.port = await findPort();
    t.context.foo = rewire('./foo');
});

test(async t => {
    await t.context.foo.createServer(t.context.port);
    t.ok(await portAvail(t.context.port));
});

And the actual files:

import http from 'http';
import config from 'app/config';
import connectToDb from 'app/db';

export async function createServer(port) {
    var server = http.createServer();
    var db = await connectToDb();
    await Promise.promisify(::server.listen)(port);
    return { server, db };
};

And I have build scripts set up so I can test, test:cov or watch based on test type:

$ npm run test
> npm run test:all

    100 tests passing.

$ npm run test:cov
> npm run test:all:cov
> ava
    100 tests passing.

    100% coverage.

$ npm run test:{unit,int,e2e}
> ...

$ npm run test:{unit,int,e2e}:cov
> ...

$ npm run watch:{unit,int,es2}
> ...
Watching...

[sindresorhus]

Then you use app-module-path for my-app, and then your tests look like this:

Just fyi. Your links never work as you use relative links.

Thanks for elaborating. I've never seen this use-case before and doesn't seem like something we should support by default. You could easily support it with a simple glob pattern. Your suggested default glob pattern of **/*.test.js wouldn't fit your use-case of webapp.test.e2e.js anyways.

If you feel strongly about it can you move this into a separate issue? It's out of scope of this PR and I don't want that discussion to block this change.

[ariporad]

@sindresorhus: Ok, Sure. It's not really that big a deal.

[jamestalmage]

@ariporad - please see the edited description and title. All your PR's should contain a quick summary of the change, and a link back to the original discussion.

[sindresorhus]

and a link back to the original discussion.

In the format Fixes #234234, so the original issue is closed when the PR is merged.

[ariporad]

@jamestalmage: Sorry, I usually do. Thanks!

[jamestalmage]

In the format Fixes ...

My understanding was that the fix was split over a couple PR's. Do you still want that syntax for partial fixes?

[ariporad]

@jamestalmage: The other one was significantly smaller, and had already been merged. So I made that one Refs #XXX and this one Fixes #XXX.

[jamestalmage]

So I made that one Refs #XXX and this one Fixes #XXX.

Works for me!

[ariporad]

@sindresorhus, @vdemedes, @jamestalmage: Anyone have any objections to merging?

[vadimdemedes]

Looks good to me!

@ariporad could you squash your commits into one?

[sindresorhus]

LGTM when squashed.

[ariporad]

Ok:

• Commits Squashed
• 2 LGTMs
  • LGTM
  • LGTM
• Builds Passing
  • Travis
  • AppVeyor
  • Coveralls
• Merged.

[sindresorhus]

Good stuff @ariporad :)

[vadimdemedes]

Thank you!