Skip to content

Deeply recurse directories #378

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.

Sign up for GitHub

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 1 commit into from
Dec 28, 2015
Merged

Deeply recurse directories #378

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions api.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -195,7 +195,7 @@ function handlePaths(files) {
files = [
'test.js',
'test-*.js',
'test/*.js'
'test'
Copy link
Contributor

@jamestalmage jamestalmage Dec 27, 2015

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@ariporad ariporad Dec 27, 2015

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@jamestalmage jamestalmage Dec 27, 2015

Choose a reason for hiding this comment

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

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.

Copy link
Member

@sindresorhus sindresorhus Dec 27, 2015

Choose a reason for hiding this comment

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

@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?

Copy link
Contributor Author

@ariporad ariporad Dec 27, 2015

Choose a reason for hiding this comment

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

@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

Copy link
Contributor Author

@ariporad ariporad Dec 27, 2015

Choose a reason for hiding this comment

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

@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).

Copy link
Member

@sindresorhus sindresorhus Dec 27, 2015

Choose a reason for hiding this comment

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

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 :)

Copy link
Contributor Author

@ariporad ariporad Dec 28, 2015

Choose a reason for hiding this comment

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

@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...

Copy link
Member

@sindresorhus sindresorhus Dec 28, 2015

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@ariporad ariporad Dec 28, 2015

Choose a reason for hiding this comment

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

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

];
}

Expand All @@ -209,7 +209,7 @@ function handlePaths(files) {
return files
.map(function (file) {
if (fs.statSync(file).isDirectory()) {
return handlePaths([path.join(file, '*.js')]);
return handlePaths([path.join(file, '**', '*.js')]);
}

return file;
Expand Down
3 changes: 2 additions & 1 deletion cli.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,11 +48,12 @@ var cli = meow([
' ava',
' ava test.js test2.js',
' ava test-*.js',
' ava test',
' ava --init',
' ava --init foo.js',
'',
'Default patterns when no arguments:',
'test.js test-*.js test/*.js'
'test.js test-*.js test/**/*.js'
], {
string: [
'_',
Expand Down
5 changes: 3 additions & 2 deletions readme.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,14 +115,15 @@ $ ava --help
ava
ava test.js test2.js
ava test-*.js
ava test
ava --init
ava --init foo.js

Default patterns when no arguments:
test.js test-*.js test/*.js
test.js test-*.js test/**/*.js
```

Files in directories named `fixtures` and `helpers` are ignored, as well as files starting with `_`. This can be useful for having helpers in the same directory as your test files.
Directories are recursive by default. Files in directories named `fixtures` and `helpers` are ignored, as well as files starting with `_`. This can be useful for having helpers in the same directory as your test files.

*WARNING: NON-STANDARD BEHAVIOR:* The AVA CLI will always try to find and use your projects local install of AVA. This is true even when you run the global `ava` command. This non-standard behavior solves an important [issue](https://github.com/sindresorhus/ava/issues/157), and should have no impact on everyday use.

Expand Down
12 changes: 12 additions & 0 deletions test/api.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -238,6 +238,18 @@ test('absolute paths', function (t) {
});
});

test('search directories recursivly for files', function (t) {
t.plan(1);

var api = new Api([path.join(__dirname, 'fixture/subdir')]);

api.run()
.then(function () {
t.is(api.passCount, 2);
t.is(api.failCount, 1);
});
});

test('titles of both passing and failing tests and AssertionErrors are returned', function (t) {
t.plan(3);

Expand Down
5 changes: 5 additions & 0 deletions test/fixture/subdir/failing-subdir.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
import test from '../../../';

test('subdir fail', t => {
t.fail();
});
5 changes: 5 additions & 0 deletions test/fixture/subdir/in-a-subdir.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
import test from '../../../';

test('subdir', t => {
t.pass();
});
5 changes: 5 additions & 0 deletions test/fixture/subdir/nested/nested.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
import test from '../../../../';

test('subdir', t => {
t.pass();
});