Skip to content

Snapshot location #1489

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 16 commits into from
Sep 2, 2017
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
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: 4 additions & 0 deletions docs/recipes/typescript.md
Original file line number Diff line number Diff line change
Expand Up @@ -83,3 +83,7 @@ test('an actual test', t => {
```
$ npm test
```

## Snapshots

Ava derives the base location for the snapshot fixtures relative to the typescript as if it were standard javascript tests. It determines this from the associated `*.js.map` sourcemap files. Enabling sourcemaps with the typescript compiler can be done in either the [tsconfig](http://www.typescriptlang.org/docs/handbook/tsconfig-json.html) `tsconfig.json#compilerOptions#sourceMap=true` or in the `tsc` cli option `--sourceMap`.
Copy link
Member

Choose a reason for hiding this comment

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

Rather than this paragraph, update the sample tsconfig.json to enable source maps. Honestly it should always be enabled, since it helps AVA show correct stack traces and code snippets when assertions fail.

1 change: 1 addition & 0 deletions lib/cli.js
Original file line number Diff line number Diff line change
Expand Up @@ -144,6 +144,7 @@ exports.run = () => {
timeout: conf.timeout,
concurrency: conf.concurrency ? parseInt(conf.concurrency, 10) : 0,
updateSnapshots: conf.updateSnapshots,
snapshotLocation: conf.snapshotLocation,
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps resolve the absolute path here: conf.snapshotLocation ? path.resolve(projectDir, conf.snapshotLocation) : null. This would even enable absolute paths, and remove complexity in the snapshot manager code.


Total bikeshed, but how do you feel about snapshotDir? Seems snappier.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure no preference here from me projectDir, I think I saw Dir used somewhere else :)

color: conf.color
});

Expand Down
3 changes: 2 additions & 1 deletion lib/main.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,8 @@ const runner = new Runner({
match: opts.match,
projectDir: opts.projectDir,
serial: opts.serial,
updateSnapshots: opts.updateSnapshots
updateSnapshots: opts.updateSnapshots,
snapshotLocation: opts.snapshotLocation
});

worker.setRunner(runner);
Expand Down
4 changes: 3 additions & 1 deletion lib/runner.js
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ class Runner extends EventEmitter {
this.projectDir = options.projectDir;
this.serial = options.serial;
this.updateSnapshots = options.updateSnapshots;
this.snapshotLocation = options.snapshotLocation;

this.hasStarted = false;
this.results = [];
Expand Down Expand Up @@ -187,7 +188,8 @@ class Runner extends EventEmitter {
projectDir: this.projectDir,
relFile: path.relative(this.projectDir, this.file),
testDir: path.dirname(this.file),
updating: this.updateSnapshots
updating: this.updateSnapshots,
location: this.snapshotLocation
Copy link
Member

Choose a reason for hiding this comment

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

If this.snapshotLocation is either null or an absolute path, this could be renamed from location to fixedLocation.

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 I like it.

});
this.emit('dependency', this.snapshots.snapPath);
}
Expand Down
48 changes: 46 additions & 2 deletions lib/snapshot-manager.js
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,18 @@ function tryRead(file) {
}
}

function tryReadSourceMap(file) {
const content = tryRead(file);
if (content) {
try {
return JSON.parse(content.toString());
} catch (err) {
console.error('Parse error on snapshot sourcemap', file);
throw err;
}
}
}

function withoutLineEndings(buffer) {
let newLength = buffer.byteLength - 1;
while (buffer[newLength] === 0x0A || buffer[newLength] === 0x0D) {
Expand Down Expand Up @@ -355,7 +367,16 @@ class Manager {
}
}

function determineSnapshotDir(projectDir, testDir) {
function determineSnapshotDir(options) {
const projectDir = options.projectDir;
const testDir = determineSourceMappedDir(options);
if (options.location) {
// "snapshotLocation" in ava package.json config, snapshots derive a location
// relative from the source testDir not the preset "__snapshots__" folder names
Copy link
Member

Choose a reason for hiding this comment

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

When options.location becomes options.fixedLocation this comment becomes unnecessary since it's clear what is expected of the code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, I guess the comment highlighted some of the ambiguitiy, please checkout the latest revision.

const resolvedLocation = path.resolve(options.projectDir, options.location);
const relativeTestLocation = options.testDir.replace(options.projectDir, '');
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this refer to testDir, rather than options.testDir?

Also, you should be able to use path.relative(projectDir, testDir) 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, please checkout the latest revision.

return path.join(resolvedLocation, relativeTestLocation);
}
const parts = new Set(path.relative(projectDir, testDir).split(path.sep));
if (parts.has('__tests__')) {
return path.join(testDir, '__snapshots__');
Expand All @@ -365,8 +386,31 @@ function determineSnapshotDir(projectDir, testDir) {
return testDir;
}

function determineSourceMappedDir(options) {
const sourceMapFilePath = path.join(options.testDir, `${options.name}.map`);
const sourceMapContent = tryReadSourceMap(sourceMapFilePath);
Copy link
Member

Choose a reason for hiding this comment

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

Use https://www.npmjs.com/package/convert-source-map, like in nyc: https://github.com/istanbuljs/nyc/blob/fb3ab927796963379edfddd4d2385002fa236f65/lib/source-maps.js#L16

This will support inline source maps as well as source map references. It does presume that these references are in the source file, but I think that's a fair assumption.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed thanks a bunch I am new to that library very cool. Seems to work well with the inline version in my testing https://github.com/impaler/ava-snapshot-location/tree/master/inline-sourcemaps


if (sourceMapContent) {
const targetFilename = path.basename(sourceMapContent.file, 'js');
const testFileSource = sourceMapContent.sources.find(file => {
const fileName = file.replace(path.extname(targetFilename), '');
return fileName.indexOf(targetFilename) > -1;
});
Copy link
Member

Choose a reason for hiding this comment

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

This logic isn't quite right. According to the source map "spec" file is optional. There might be a sourceRoot which should be prepended to the sources.

I couldn't quite find a library which exports the correct source resolution. I think this will be sufficient for our use case (though I haven't tested it):

// Assume there is but a single source. Multiple sources imply multiple possible
// snapshot locations, which won't work.
const firstSource = sourceMapContent.sources[0];
const rootedSource = sourceMapContent.sourceRoot ?
  sourceMapContent.sourceRoot + firstSource :
  firstSource;
// `convert-source-map` doesn't provide the path to the source map file, if the
// source map came from such a file. Assuming that if it did, it is stored right
// next to the test file. Or, alternatively, if it is stored elsewhere,
// resolving `rootedSource` relative to the test file results in a path that
// is the same as if it had been resolved relative to the source map file.
// Note that `options.relFile` is relative to `options.projectDir`.
const sourceFile = path.resolve(path.join(options.projectDir, options.relFile), rootedSource);
return path.dirname(sourceFile);

I'm not sure how much we need to guard against malformed source maps (e.g. no or empty sources), or source maps that unexpectedly resolve to an HTTP source.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks I learned a few things here. I guess we wont be able to answer how much more to guard without trying more than just typescript. With the way things are layed out it shouldn't be too hard to expand on this.

if (testFileSource) {
if (options.location) {
const basePath = path.join(options.projectDir, options.location);
const sourceTestFilePath = path.dirname(path.resolve(options.testDir, testFileSource));
const relativePath = sourceTestFilePath.replace(options.projectDir, '');
return path.join(basePath, relativePath);
}
return path.dirname(path.resolve(options.testDir, testFileSource));
}
}
return options.testDir;
}

function load(options) {
const dir = determineSnapshotDir(options.projectDir, options.testDir);
const dir = determineSnapshotDir(options);
const reportFile = `${options.name}.md`;
const snapFile = `${options.name}.snap`;
const snapPath = path.join(dir, snapFile);
Expand Down
14 changes: 14 additions & 0 deletions readme.md
Original file line number Diff line number Diff line change
Expand Up @@ -1041,6 +1041,20 @@ You can then check your code. If the change was intentional you can use the `--u
$ ava --update-snapshots
```

You can also set a custom location for the generated snapshot fixtures. This can be configured in the `package.json#ava` configuration for every test run.
Copy link
Member

Choose a reason for hiding this comment

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

I'd prefer:

You can specify a fixed location for storing the snapshot files from each test under the "ava" key in your package.json:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍


```json
{
"ava": {
"snapshotLocation": "custom-directory"
}
}
```

Snapshots fixtures will be then saved in a directory structure that reflects the test sourcecode.
Copy link
Member

Choose a reason for hiding this comment

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

The snapshot files will be saved in a directory structure that mirrors that of your test files.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍


If you are transpiling the tests with sourcemaps, ava will derive the test location from the `*.js.map` files, see more in the [TypeScript](docs/recipes/typescript.md) recipe.
Copy link
Member

Choose a reason for hiding this comment

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

If you are running AVA against precompiled test files, AVA will try and use source maps to determine the location of the original files. Snapshots will be stored next to these files, following the same rules as if AVA had executed the original files directly. This is great if you're writing your tests in TypeScript (see our TypeScript recipe).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍


### Skipping assertions

Any assertion can be skipped using the `skip` modifier. Skipped assertions are still counted, so there is no need to change your planned assertion count.
Expand Down
43 changes: 43 additions & 0 deletions test/cli.js
Original file line number Diff line number Diff line change
Expand Up @@ -680,6 +680,49 @@ test('legacy snapshot files are reported to the console', t => {
});
});

test('snapshots infer their location from sourcemaps', t => {
t.plan(8);
const dirname = path.join('fixture/snapshots/test-sourcemaps');
const testLocationTypes = [
'src',
'src/test/snapshots',
'src/feature/__tests__/__snapshots__'
];
const removeExists = relFilePath => {
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't remove anything though?

You should remove these files at least prior to running the test, otherwise on local setups you won't be able to tell if the code has broken if they're persisted from previous runs.

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 this is wrong, I have updated this be a little more dry 12f1ff8#diff-583d616f4b01bf1fa153e71c0cef7de7R704. If there are more integration tests on projects like this I can think of some better ways to do it, like maybe the project should be first duplicated to a /tmp location and so on.

Copy link
Member

Choose a reason for hiding this comment

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

I'm not too concerned about how DRY this is. The other test looks good, just need to take that same approach here.

const snapshotFolderPath = path.join(__dirname, dirname, relFilePath);
t.true(fs.existsSync(path.join(snapshotFolderPath, 'test.js.md')));
t.true(fs.existsSync(path.join(snapshotFolderPath, 'test.js.snap')));
};
execCli([], {dirname}, (err, stdout, stderr) => {
t.ifError(err);
testLocationTypes.forEach(removeExists);
t.match(stderr, /6 passed/);
t.end();
});
});

test('snapshots resolved location from "snapshotLocation" in ava config', t => {
t.plan(8);
const dirname = path.join('fixture/snapshots/test-snapshot-location');
const snapshotLocation = 'snapshot-fixtures';
const testLocationTypes = [
'src',
'src/feature',
'src/feature/nested-feature'
];
const removeExists = relFilePath => {
const snapshotFolderPath = path.join(__dirname, dirname, snapshotLocation, relFilePath);
t.true(fs.existsSync(path.join(snapshotFolderPath, 'test.js.md')));
t.true(fs.existsSync(path.join(snapshotFolderPath, 'test.js.snap')));
};
execCli([], {dirname}, (err, stdout, stderr) => {
t.ifError(err);
testLocationTypes.forEach(removeExists);
t.match(stderr, /6 passed/);
t.end();
});
});

test('--no-color disables formatting colors', t => {
execCli(['--no-color', '--verbose', 'formatting-color.js'], {dirname: 'fixture'}, (err, stdout, stderr) => {
t.ok(err);
Expand Down
8 changes: 8 additions & 0 deletions test/fixture/snapshots/test-snapshot-location/package.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
{
"ava": {
"files": [
"src/**/*test.js"
],
"snapshotLocation": "snapshot-fixtures"
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
import test from '../../../../../../..';

test('test nested feature title', t => {
t.snapshot({foo: 'bar'});

t.snapshot({answer: 42});
});

test('another nested feature test', t => {
t.snapshot(new Map());
});
11 changes: 11 additions & 0 deletions test/fixture/snapshots/test-snapshot-location/src/feature/test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
import test from '../../../../../..';

test('test feature title', t => {
t.snapshot({foo: 'bar'});

t.snapshot({answer: 42});
});

test('another feature test', t => {
t.snapshot(new Map());
});
11 changes: 11 additions & 0 deletions test/fixture/snapshots/test-snapshot-location/src/test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
import test from '../../../../..';

test('test title', t => {
t.snapshot({foo: 'bar'});

t.snapshot({answer: 42});
});

test('another test', t => {
t.snapshot(new Map());
});

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

9 changes: 9 additions & 0 deletions test/fixture/snapshots/test-sourcemaps/build/test.js

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions test/fixture/snapshots/test-sourcemaps/build/test.js.map

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

9 changes: 9 additions & 0 deletions test/fixture/snapshots/test-sourcemaps/build/test/test.js

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

10 changes: 10 additions & 0 deletions test/fixture/snapshots/test-sourcemaps/package.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
{
"scripts": {
"build": "tsc"
Copy link
Member

Choose a reason for hiding this comment

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

Does this assume tsc is installed globally? I know we have a dev dependency on it but running build inside this directory won't pick it up. This should either refer to the top-level node_modules/.bin/tsc or use npx tsc.

(Also, going by the GitHub output here, the indentation seems off in this file and tsconfig.json.)

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 it does, I don't think there will be any need to run build here for the tests anyway. So I fixed the indentation and removed "scripts".

},
"ava": {
"files": [
"build/**/*test.js"
]
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
import test from '../../../../../../..'

test('feature test title', t => {
t.snapshot({foo: 'bar'});

t.snapshot({answer: 40});
});

test('another feature test', t => {
t.snapshot(new Map());
});
11 changes: 11 additions & 0 deletions test/fixture/snapshots/test-sourcemaps/src/test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
import test from '../../../../..';

test('top level test title', t => {
t.snapshot({foo: 'bar'});

t.snapshot({answer: 42});
});

test('another top level test', t => {
t.snapshot(new Map());
});
11 changes: 11 additions & 0 deletions test/fixture/snapshots/test-sourcemaps/src/test/test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
import test from '../../../../../..';

test('test title', t => {
t.snapshot({foo: 'bar'});

t.snapshot({answer: 43});
});

test('another test', t => {
t.snapshot(new Map());
});
11 changes: 11 additions & 0 deletions test/fixture/snapshots/test-sourcemaps/tsconfig.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
{
"compilerOptions": {
"target": "es6",
"module": "commonjs",
"outDir": "build",
"sourceMap": true
},
"include": [
"src/**/*test.ts"
]
}