Expose snapshot directory through test.meta #2444

ulken · 2020-04-07T23:05:29Z

Closes #1984

IssueHunt Summary

Referenced issues

This pull request has been submitted to:

#1984: Expose config object

IssueHunt has been backed by the following sponsors. Become a sponsor

novemberborn

Nice!

Let's make snapshotDirectory a getter. We don't need to resolve it in test files that don't use snapshots.

Regarding the CI failures, I think you need to add mem to this list, since it messes with some expected stack traces:

ava/lib/beautify-stack.js

Line 10 in 447d371

    
           const avaDependencies = /\/node_modules\/(?:@ava\/babel|@ava\/require-precompiled|append-transform|empower-core|nyc|require-precompiled|(?:ava\/node_modules\/)?(?:babel-runtime|core-js))\//;

(We're trying to clean this up: #2420)

There's some other failures due to perhaps some tests that are a little too low level, and are now breaking. I haven't looked into it much yet.

lib/runner.js

ulken · 2020-04-08T13:05:50Z

Let's make snapshotDirectory a getter. We don't need to resolve it in test files that don't use snapshots.

Ah, that's what you meant with getter – smart! I was looking for a way to only compute the path if snapshots was actually used. First turned to options.snapshotDir, but then learned that was just used as a "prefix". A lazy getter would solve it just perfectly.

Regarding the CI failures, I think you need to add mem to this list, since it messes with some expected stack traces:

ava/lib/beautify-stack.js

Line 10 in 447d371

const avaDependencies = /\/node_modules\/(?:@ava\/babel|@ava\/require-precompiled|append-transform|empower-core|nyc|require-precompiled|(?:ava\/node_modules\/)?(?:babel-runtime|core-js))\//;

Thanks, wasn't aware. You might be right about that. I'll try it out. I was being lazy/too tired to run the whole test suite on my machine. Shouldn't have created the final PR (draft could have worked). Sorry about that.

There's some other failures due to perhaps some tests that are a little too low level, and are now breaking. I haven't looked into it much yet.

Yeah, I think I might be using mem wrongly. I'm having a hard time debugging tap tests, though. console.log won't output anything (swallowed?). Can you run tap in debug mode with breakpoints in VS Code, do you know?

Co-Authored-By: Mark Wubben <[email protected]>

ulken · 2020-04-08T13:16:12Z

Also, is there no feasible way to only run one test with tap? Like AVA's t.only()? I looked at the docs, but it was so tedious...

novemberborn · 2020-04-08T13:49:48Z

Also, is there no feasible way to only run one test with tap? Like AVA's t.only()? I looked at the docs, but it was so tedious...

npx tap test.js --grep=title or something like that. Or you can add {only: true} as the final argument, I think.

lib/runner.js

novemberborn · 2020-04-08T13:51:52Z

I'm having a hard time debugging tap tests, though. console.log won't output anything (swallowed?). Can you run tap in debug mode with breakpoints in VS Code, do you know?

Try console.error(). I've never actually tried to debug tap tests.

ulken · 2020-04-08T17:34:57Z

Try console.error(). I've never actually tried to debug tap tests.

Will do, thanks! So your theory is that console.error() is not swallowed?

ulken · 2020-04-08T17:36:36Z

npx tap test.js --grep=title or something like that. Or you can add {only: true} as the final argument, I think.

oO, looks promising. I’ll give it a go. Thanks!

ulken · 2020-04-08T20:24:07Z

npx tap test.js --grep=title or something like that. Or you can add {only: true} as the final argument, I think.

Follow-up: --grep worked fine. The optional option object ({only: true}) is passed as the second parameter, not the third/last (the callback comes last). Also, tap --only is required to only run "only" tests. Good to know, thanks!

ulken · 2020-04-08T20:29:02Z

Try console.error(). I've never actually tried to debug tap tests.

No go ;( Fumbling in the dark...

So I have a test fixture which uses AVA syntax. The tap test for api.js runs that file. Any logs, throws etc. inside the fixture file don't show up in the terminal. Any ideas?

ulken · 2020-04-08T21:00:30Z

Looks like I don't have to mess with beautify-stack/avaDependencies after all. At least all tests pass locally. Still have to await CI...

ulken · 2020-04-08T21:05:57Z

lib/snapshot-manager.js

+	const dir = determineSnapshotDir({file, fixedLocation, projectDir});
+	const relFile = path.relative(projectDir, resolveSourceFile(file));


Note: In order to not have to export both determineSnapshotDir() and resolveSourceFile() I changed the signature of determineSnapshotDir() to take the "raw" file instead of the source file (it resolves the source file on its own).

Caveat: resolveSourceFile() is called twice for load() (once more to determine the relative path). But since it's memoized, I think it should be fine, no?

I think that's fine.

ulken · 2020-04-08T21:08:03Z

lib/runner.js

@@ -54,7 +54,11 @@ class Runner extends Emittery {
 		let hasStarted = false;
 		let scheduledStart = false;
 		const meta = Object.freeze({
-			file: options.file
+			file: this.file,


Doesn't really matter, I guess, but should I revert this to use options again? I switched to this since I think that's what's being used elsewhere, but now it looks kinda inconsistent when snapshotDirectory() is using options. Opinion?

Yea I'll change it back.

ulken · 2020-04-08T21:09:52Z

test-tap/fixture/meta.js

+test('meta is test.meta', t => {
+	t.is(meta, test.meta);
+});


Is this test necessary? I added it to not have to add two tests for every property as was done for file (checking both meta.property and test.meta.property).

I guess it won't hurt 😄

I'm looking to remove this extra export in #2435.

ulken · 2020-04-08T21:10:08Z

test-tap/fixture/meta.js

-test('test.meta.file', t => {
-	t.is(test.meta.file, __filename);
+test('meta.snapshotDirectory', t => {
+	t.regex(meta.snapshotDirectory, /.*snapshot-fixture.*/);


Regex specific enough?

Yea it's fine. Though you don't need the .*.

ulken · 2020-04-08T21:12:14Z

docs/01-writing-tests.md


-Helper files can determine the filename of the test being run by reading `test.meta.file`.  This eliminates the need to pass `__filename` from the test to helpers.
+Access metadata of the test being run by reading `test.meta`.


I tried to keep this short. Found the previous text a little redundant.

Yea… helper files are less emphasized within AVA core anyhow.

ulken · 2020-04-08T21:13:14Z

@novemberborn Okidoki, I think we're good to go here. Added some self-review comments.

novemberborn

Great! I've pushed some tweaks, let's see if I didn't break CI.

Anything else from your end?

novemberborn · 2020-04-10T14:12:22Z

docs/01-writing-tests.md


-Helper files can determine the filename of the test being run by reading `test.meta.file`.  This eliminates the need to pass `__filename` from the test to helpers.
+Access metadata of the test being run by reading `test.meta`.


Yea… helper files are less emphasized within AVA core anyhow.

novemberborn · 2020-04-10T14:13:44Z

lib/runner.js

@@ -54,7 +54,11 @@ class Runner extends Emittery {
 		let hasStarted = false;
 		let scheduledStart = false;
 		const meta = Object.freeze({
-			file: options.file
+			file: this.file,


Yea I'll change it back.

novemberborn · 2020-04-10T14:13:56Z

lib/snapshot-manager.js

+	const dir = determineSnapshotDir({file, fixedLocation, projectDir});
+	const relFile = path.relative(projectDir, resolveSourceFile(file));


I think that's fine.

novemberborn · 2020-04-10T14:14:38Z

test-tap/fixture/meta.js

+test('meta is test.meta', t => {
+	t.is(meta, test.meta);
+});


I guess it won't hurt 😄

I'm looking to remove this extra export in #2435.

novemberborn · 2020-04-10T14:15:03Z

test-tap/fixture/meta.js

-test('test.meta.file', t => {
-	t.is(test.meta.file, __filename);
+test('meta.snapshotDirectory', t => {
+	t.regex(meta.snapshotDirectory, /.*snapshot-fixture.*/);


Yea it's fine. Though you don't need the .*.

ulken · 2020-04-10T14:19:46Z

LGTM!

ulken added 8 commits April 7, 2020 14:57

✏️ meta data -> metadata

d9723d8

✏️ test -> test file

2ae1e64

✏️ Trim whitespace

e5dc8b0

📝 Update docs

0e90020

✏️ Trim whitespace

3861b14

🏷️ Update Meta type

7ec16e1

✨ Expose meta.snapshotDirectory

3eefd81

✨ Add meta.snapshotDirectory

86d3a3d

ulken mentioned this pull request Apr 7, 2020

Expose config object #1984

Closed

Minor tweaks

24aa738

novemberborn requested changes Apr 8, 2020

View reviewed changes

lib/runner.js Outdated Show resolved Hide resolved

⚡ Make snapshotDirectory a getter

d324507

Co-Authored-By: Mark Wubben <[email protected]>

ulken added 2 commits April 8, 2020 15:27

Fix mem cacheKey usage

5f89a13

🐛 Fix get syntax

72182da

novemberborn reviewed Apr 8, 2020

View reviewed changes

lib/runner.js Outdated Show resolved Hide resolved

🐛 this -> options

31219da

ulken commented Apr 8, 2020

View reviewed changes

Final tweaks

c164be8

novemberborn approved these changes Apr 10, 2020

View reviewed changes

novemberborn changed the title ~~Expose meta.snapshotDirectory~~ Expose snapshot directory through test.meta Apr 10, 2020

novemberborn merged commit cb5f9f7 into avajs:master Apr 10, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expose snapshot directory through test.meta #2444

Expose snapshot directory through test.meta #2444

ulken commented Apr 7, 2020 •

edited by issuehunt-oss bot

Loading

novemberborn left a comment

ulken commented Apr 8, 2020

ulken commented Apr 8, 2020

novemberborn commented Apr 8, 2020

novemberborn commented Apr 8, 2020

ulken commented Apr 8, 2020

ulken commented Apr 8, 2020

ulken commented Apr 8, 2020

ulken commented Apr 8, 2020

ulken commented Apr 8, 2020

ulken Apr 8, 2020

novemberborn Apr 10, 2020

ulken Apr 8, 2020

novemberborn Apr 10, 2020

ulken Apr 8, 2020

novemberborn Apr 10, 2020

ulken Apr 8, 2020

novemberborn Apr 10, 2020

ulken Apr 8, 2020 •

edited

Loading

novemberborn Apr 10, 2020

ulken commented Apr 8, 2020

novemberborn left a comment

novemberborn Apr 10, 2020

novemberborn Apr 10, 2020

novemberborn Apr 10, 2020

novemberborn Apr 10, 2020

novemberborn Apr 10, 2020

ulken commented Apr 10, 2020

		const dir = determineSnapshotDir({file, fixedLocation, projectDir});
		const relFile = path.relative(projectDir, resolveSourceFile(file));


		Helper files can determine the filename of the test being run by reading `test.meta.file`. This eliminates the need to pass `__filename` from the test to helpers.
		Access metadata of the test being run by reading `test.meta`.

Expose snapshot directory through test.meta #2444

Expose snapshot directory through test.meta #2444

Conversation

ulken commented Apr 7, 2020 • edited by issuehunt-oss bot Loading

Referenced issues

novemberborn left a comment

Choose a reason for hiding this comment

ulken commented Apr 8, 2020

ulken commented Apr 8, 2020

novemberborn commented Apr 8, 2020

novemberborn commented Apr 8, 2020

ulken commented Apr 8, 2020

ulken commented Apr 8, 2020

ulken commented Apr 8, 2020

ulken commented Apr 8, 2020

ulken commented Apr 8, 2020

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

ulken Apr 8, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ulken commented Apr 8, 2020

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

ulken commented Apr 10, 2020

ulken commented Apr 7, 2020 •

edited by issuehunt-oss bot

Loading

ulken Apr 8, 2020 •

edited

Loading