Snapshot location #1489

impaler · 2017-08-13T02:26:39Z

@novemberborn As discussed in #1459 the proposal to support custom snapshot locations and sourcemaps is:

Check if the test file has a source map associated with it, and attempt to resolve the source file path
Have that source file path take precedence over the test file path when it comes to loading and saving the snapshot
Add a setting in package.json#ava to force snapshots to be stored in the specified global directory. This takes precedence over the source file detection
When snapshots are stored in a global directory, mirror the file hierarchy of the test file (with the source file path still taking precedence) inside the global directory
We should also add an integration test where the test file comes from a TypeScript compiler

Although there are some the test fixtures added for integration tests, I created an example project to more easily test the implementation.
https://github.com/impaler/ava-snapshot-location

…hot location through sourcemaps. As discussed in avajs#1459

- throw on invalid sourcemap - remove extra snapshot-location cli option

…s#1459

…ipt recipe.

novemberborn

Thanks for the PR @impaler! I'm excited about seeing both the source map resolution land, as well as having a configurable snapshot directory.

novemberborn · 2017-08-13T12:38:52Z

lib/cli.js

@@ -144,6 +144,7 @@ exports.run = () => {
 		timeout: conf.timeout,
 		concurrency: conf.concurrency ? parseInt(conf.concurrency, 10) : 0,
 		updateSnapshots: conf.updateSnapshots,
+		snapshotLocation: conf.snapshotLocation,


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.

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

novemberborn · 2017-08-13T12:43:42Z

lib/runner.js

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


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

Yes I like it.

novemberborn · 2017-08-13T12:44:17Z

lib/snapshot-manager.js

+	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


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

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

novemberborn · 2017-08-13T12:45:55Z

lib/snapshot-manager.js

+		// "snapshotLocation" in ava package.json config, snapshots derive a location
+		// relative from the source testDir not the preset "__snapshots__" folder names
+		const resolvedLocation = path.resolve(options.projectDir, options.location);
+		const relativeTestLocation = options.testDir.replace(options.projectDir, '');


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

Also, you should be able to use path.relative(projectDir, testDir) here.

Yes, please checkout the latest revision.

novemberborn · 2017-08-13T12:48:36Z

lib/snapshot-manager.js

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


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.

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

novemberborn · 2017-08-13T13:38:49Z

readme.md

+
+Snapshots fixtures will be then saved in a directory structure that reflects the test sourcecode.
+
+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.


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

novemberborn · 2017-08-13T13:41:09Z

test/fixture/snapshots/test-sourcemaps/build/feature/__tests__/test.js

+test('another feature test', t => {
+    t.snapshot(new Map());
+});
+//# sourceMappingURL=test.js.map


Could you make sure these files have a newline at the end of them?

Is that just for constency? in this case everything under ...build/* is directly from the typescript compiler, I usually hesitate to edit compiled files for style only.

Fair enough.

novemberborn · 2017-08-13T13:41:54Z

test/cli.js

+		'src/test/snapshots',
+		'src/feature/__tests__/__snapshots__'
+	];
+	const removeExists = relFilePath => {


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.

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.

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

novemberborn · 2017-08-13T13:44:49Z

test/fixture/snapshots/test-sourcemaps/package.json

@@ -0,0 +1,10 @@
+{
+  "scripts": {
+	"build": "tsc"


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

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

novemberborn · 2017-08-13T13:47:43Z

docs/recipes/typescript.md

+
+## 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`.


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.

impaler · 2017-08-16T11:04:18Z

Thanks for the great review @novemberborn :)

I won't claim that we have complete integration test coverage on this snapshot feature, but it has the main aspects. Otherwise as I said in a comment I think we would need some more utilities to duplicate project folders and run all scenarios like I am testing in https://github.com/impaler/ava-snapshot-location.

Based on what I can see on appveyor the ci issues seem unrelated.

novemberborn

Very cool, I think this is almost there!

Based on what I can see on appveyor the ci issues seem unrelated.

Yea. I'm rerunning it just in case.

I won't claim that we have complete integration test coverage on this snapshot feature, but it has the main aspects. Otherwise as I said in a comment I think we would need some more utilities to duplicate project folders and run all scenarios like I am testing in impaler/ava-snapshot-location.

I won't say no to a PR that achieves that, but it doesn't need to hold up landing this one. All new lines are covered so that's 💯 https://codecov.io/gh/avajs/ava/pull/1489/diff

novemberborn · 2017-08-20T15:07:06Z

lib/snapshot-manager.js

-		const relativeTestLocation = options.testDir.replace(options.projectDir, '');
-		return path.join(resolvedLocation, relativeTestLocation);
+	if (options.fixedLocation) {
+		const relativeTestLocation = testDir.replace(options.projectDir, '');


Use path.relative(options.projectDir, testDir).

novemberborn · 2017-08-20T15:09:07Z

lib/snapshot-manager.js

-			}
-			return path.dirname(path.resolve(options.testDir, testFileSource));
-		}
+	const sourceMapContent = tryRead(options.relFile).toString();


tryRead() returns null if the file doesn't exist. You'll have to read it into a variable and bail out of if it's null.

Perhaps this function should return null if no source-mapped directory could be determined. Then determineSnapshotDir() can do testDir = determineSourceMappedDir(options) || options.testDir.

novemberborn · 2017-08-20T15:13:36Z

test/cli.js

+		})
+		.reduce((a, b) => a.concat(b), []);
+	const removeExistingSnapshotFixtureFiles = filePath => {
+		return fs.existsSync(filePath) && fs.unlinkSync(filePath);


There is still room for the file to be deleted right in between existsSync and unlinkSync. Would be better to do https://github.com/impaler/ava/blob/12f1ff8d73e0ae47c109eed9737470c9d421fa8e/test/cli.js#L568:L574, even if it's more verbose.

Sure that will also throw any fs issues, I updated the pull request :)

novemberborn · 2017-08-20T15:17:42Z

test/cli.js

+		'src/test/snapshots',
+		'src/feature/__tests__/__snapshots__'
+	];
+	const removeExists = relFilePath => {


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

novemberborn · 2017-08-20T15:18:38Z

test/fixture/snapshots/test-snapshot-location/package.json

+			"src/**/*test.js"
+		],
+	"snapshotDir": "snapshot-fixtures"
+	}


Nitpick alert: the indentation in this file is inconsistent. It's just a fixture, but once seen I can't not comment 😉

novemberborn · 2017-08-20T15:19:22Z

test/fixture/snapshots/test-sourcemaps/build/feature/__tests__/test.js

+test('another feature test', t => {
+    t.snapshot(new Map());
+});
+//# sourceMappingURL=test.js.map


Fair enough.

…ck. Rename local var prefix from snapshot to snap for "snappyness".

impaler · 2017-08-23T05:25:42Z

The windoze build still seems intermittent and unrelated, since the same error is happening in other branches it seems out of scope.

Let me know if the github squash commits isn't enough, Cheers :)

Set to null, since the location behavior is verified through CLI tests.

* Pass the absolute test file path into the snapshot manager * Read the test file using the absolute path (otherwise the read would be relative to the process' working directory) * Pass test directory when reading from map file source * Correctly resolve source file from the test directory * Remove incorrect '../' source root default value * Rename variables for better clarity * Remove ternaries

novemberborn · 2017-08-27T16:36:14Z

@impaler this is great! I've pushed some minor fixes, and some larger fixes to determineSourceMappedDir(). See the commits.

Could you give those a spin with your project? Will merge this if it's all good.

impaler · 2017-09-01T02:23:25Z

Thanks @novemberborn, yes everything is working as I expected with your changes :)

And looks like appveyor is now working, know if it was it related to changes here?

novemberborn · 2017-09-02T16:27:07Z

And looks like appveyor is now working, know if it was it related to changes here?

There are some known issues, e.g. #1427. But I've been having loads of problems in #1499 too. So 🤷‍♂️.

novemberborn · 2017-09-02T16:36:53Z

Thanks @impaler!

* Allow users to configure the snapshot location * Resolve the location through source-maps, in case the tests were precompiled into a build directory, e.g. via TypeScript

impaler added 6 commits July 21, 2017 16:48

Add ability for a custom snapshot location and resolving of the snaps…

f2471c8

…hot location through sourcemaps. As discussed in avajs#1459

avajs#1459 Resolve custom location for non sourcemap tests.

2c344fa

- throw on invalid sourcemap - remove extra snapshot-location cli option

Add snapshot integration tests for location & sourcemap features avaj…

5c65c9e

…s#1459

fix ava config avajs#1459

9e98219

(doc) snapshotLocation configuration and sourcemap support in typescr…

548c6b6

…ipt recipe.

Add tsconfig.json to reproduce sourcemap snapshot fixture.

d239882

novemberborn requested changes Aug 13, 2017

View reviewed changes

impaler added 2 commits August 16, 2017 20:23

(refactor) based on feedback, now uses 'convert-source-map' avajs#1489

12f1ff8

(style) fix indentation, remove build script.

902f0d3

novemberborn requested changes Aug 20, 2017

View reviewed changes

impaler added 2 commits August 23, 2017 13:49

(style) fix indentation.

3ec0217

(refactor) update snapshot location integration tests based on feedba…

e3fc508

…ck. Rename local var prefix from snapshot to snap for "snappyness".

novemberborn added 6 commits August 27, 2017 16:51

Keep snapshot manager options in alphabetical order

6282999

Add fixedLocation option when loading snapshot manager in tests

de453b3

Set to null, since the location behavior is verified through CLI tests.

Use path.relative() rather than string-replacing path prefixes

22838a9

Touch up README

61af5da

Correct AVA capitalization in test title

982a0a2

novemberborn approved these changes Aug 27, 2017

View reviewed changes

novemberborn merged commit 7fadc34 into avajs:master Sep 2, 2017

kevva pushed a commit that referenced this pull request Sep 13, 2017

Improve where snapshots are stored (#1489)

157a0be

* Allow users to configure the snapshot location * Resolve the location through source-maps, in case the tests were precompiled into a build directory, e.g. via TypeScript

masaeedu mentioned this pull request Oct 1, 2017

Snapshot fixture location with transpiled tests #1459

Closed

Marsup mentioned this pull request Feb 25, 2020

Expose test file path within test process. #1977

Merged


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

		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.


		## 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`.

Snapshot location #1489

Snapshot location #1489

Conversation

impaler commented Aug 13, 2017 • edited Loading

novemberborn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

impaler commented Aug 16, 2017

novemberborn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

impaler commented Aug 23, 2017 • edited Loading

novemberborn commented Aug 27, 2017

impaler commented Sep 1, 2017

novemberborn commented Sep 2, 2017

novemberborn commented Sep 2, 2017

impaler commented Aug 13, 2017 •

edited

Loading

impaler commented Aug 23, 2017 •

edited

Loading