Make test file extensions configurable #1746

greym0uth · 2018-03-19T20:26:13Z

Allows for the use of extensions like jsx, ts, etc..

Fixes #631

greym0uth · 2018-03-19T20:44:30Z

Referencing #631

novemberborn

Thanks for doing the PR! It's not quite the right approach yet, see #631 (comment) for a fuller explanation.

greym0uth · 2018-03-20T13:26:40Z

Gotcha, I'll make the updates and update this PR. Thanks for the filler!

greym0uth · 2018-03-21T02:15:42Z

@novemberborn Looking at the code its a bit above my current knowledge with ava.

novemberborn · 2018-03-21T09:24:07Z

@Jaden-Giordano no worries. I'm happy to give pointers if you want?

greym0uth · 2018-03-21T14:58:32Z

@novemberborn that would be wonderful!

I was looking through the code for awhile and I understand how the pipeline is initiated. I'm just having trouble determining which tests should have the precompile ran and which tests shouldnt.

Edit:
I just noticed your comment on the issue. I guess im just trying to think of the best way to pass the extension options to the precompiler.

Edit 2:
I was able to figure out a way to pass them along and create deterministic precompilation based on extension parameters. Ill update the pr.

greym0uth · 2018-03-21T17:16:19Z

There are currently errors I'm attempting to resolve. I keep getting ResolveError: D:\Projects\OpenSource\ava-test\.babelrc: Couldn't find preset "@babel/core" relative to directory when testing ava in a project.

Edit:

There are other issues as you can see in the CI, I'm still trying to figure out why this is happening if you can help maybe figure that out. After this next steps would be the watcher side of things.

greym0uth · 2018-03-25T13:37:03Z

@novemberborn You think you can help me with that?

novemberborn · 2018-03-26T15:18:42Z

Hi @Jaden-Giordano, thanks for continuing to work on this. I've been a bit busy lately so it may take a few more days before I can get back to you on this, sorry.

greym0uth · 2018-03-26T23:17:18Z

No worries, I assumed so, thanks for being so helpful so far.

greym0uth · 2018-03-31T19:48:56Z

Update:

So I managed to move past those errors, but now the cli and api tests are not passing, Im assuming this is happening since I added two properties to the config (extensions and babel.extensions), I'll continue to look into what the root cause of these issues are, but if anyone is willing to help who is familiar with the api and cli files help/feedback would be greatly appreciated.

Next steps after fixes:

After this fix, we should be ready to move on to notifying the watch.

novemberborn

Wow nice work @Jaden-Giordano! Thanks for persisting with this. I've left some hopefully helpful comments.

novemberborn · 2018-04-01T16:47:42Z

api.js

@@ -43,6 +43,12 @@ class Api extends EventEmitter {
 		const apiOptions = this.options;
 		runtimeOptions = runtimeOptions || {};

+		const doNotCompileExtensions = apiOptions.extensions || [];
+		const babelExtensions = apiOptions.babelConfig.extensions || [];


FYI apiOptions.babelConfig can be null. See:

ava/lib/cli.js

Line 145 in 977cf4f

babelConfig: babelConfigHelper.validate(conf.babel),

and:

ava/lib/babel-config.js

Line 18 in 977cf4f

return null;

That said when it's not null it'd be good if it could have a default extensions value of ['js']. The validate() function in babel-config should take care of that.

Since these extensions won't change during test runs, perhaps compute them in the constructor?

You may still run into some CI problems since not all tests properly set up the Api instance. It'd be OK to set a default value if fixing the tests proves too much effort.

novemberborn · 2018-04-01T16:49:09Z

api.js

+		const babelExtensions = apiOptions.babelConfig.extensions || [];
+
+		// Combine all extensions possible for testing.
+		const allExts = [...doNotCompileExtensions, ...babelExtensions];


This will fail CI in Node.js 4, but don't worry about that. We're dropping support since it's pretty much end of life anyway. I just need to open the necessary issues.

novemberborn · 2018-04-01T16:53:35Z

lib/ava-files.js

@@ -115,9 +116,12 @@ class AvaFiles {
 		});

 		if (files.length === 0) {
-			files = defaultIncludePatterns();
+			files = defaultIncludePatterns(options.extensions || ['js']);


You've changed defaultIncludePatterns to take a single pattern, but here you're calling it with an array. That's probably why CI is failing. Looks like you need the this.extensionPattern value from below.

Thanks for this catch!

novemberborn · 2018-04-01T16:55:38Z

lib/ava-files.js


 				if (process.platform === 'win32') {
 					// Always use `/` in patterns, harmonizing matching across platforms
 					pattern = slash(pattern);
 				}

-				return handlePaths([pattern], excludePatterns, globOptions);
+				return handlePaths([pattern], extensions, excludePatterns, globOptions, true);


That last true parameter seems unused?

Alright I removed that.

novemberborn · 2018-04-01T17:05:20Z

lib/babel-config.js

@@ -22,7 +22,7 @@ function validate(conf) {
 		return {testOptions: {}};
 	}

-	if (!conf || typeof conf !== 'object' || !conf.testOptions || typeof conf.testOptions !== 'object' || Array.isArray(conf.testOptions) || Object.keys(conf).length > 1) {
+	if (!conf || typeof conf !== 'object' || !conf.testOptions || typeof conf.testOptions !== 'object' || Array.isArray(conf.testOptions)) {


The Object.keys() bit was there to ensure only known keys were used. So we still need that, but it's gotten a bit trickier since more than 1 key is allowed.

It should be possible to provide extensions and not testOptions.

I'll add another function that checks the keys and return a boolean for whether the keys are valid or not and place it in that checks place.

novemberborn · 2018-04-01T17:05:52Z

lib/caching-precompiler.js

@@ -30,7 +30,11 @@ class CachingPrecompiler {
 		this.transform = this._createTransform();
 	}

-	precompileFile(filePath) {
+	precompileFile(filePath, excludeExtensions) {
+		if (excludeExtensions.includes(path.extname(filePath))) {


I think this should be guarded against in the calling code.

Good idea moving this to be checked here https://github.com/avajs/ava/blob/master/api.js#L134

greym0uth · 2018-04-03T21:43:18Z

@novemberborn Thank you for the insight it helped immensely! That fixed most of the issues. It is working if I install from my git repo with this current branch and run it with babel.extensions = ['jsx'] too. Maybe you have some idea what is going on in the CI? With that being said I'm going to go ahead and move on to informing the watcher.

novemberborn · 2018-04-04T08:21:22Z

Maybe you have some idea what is going on in the CI?

A newer Babel 7 is being picked up in one of the test jobs, and it's causing test failures. I'm a little surprised at that, to be honest! Feel free to ignore the Travis run with FRESH_DEPS=true, and AppVeyor as well since it doesn't run any other tests after the fresh-deps one fails.

It's going to be a few days before I'll have a chance to look at this. Thanks for your hard work @Jaden-Giordano!

greym0uth · 2018-04-04T14:00:27Z

Sounds good, take you're time.

novemberborn

This is shaping up nicely @Jaden-Giordano!

I'm sure it's on your to-do list but we'll need some tests for the new behavior, too.

novemberborn · 2018-04-07T17:03:08Z

api.js

@@ -43,6 +43,12 @@ class Api extends EventEmitter {
 		const apiOptions = this.options;
 		runtimeOptions = runtimeOptions || {};

+		const doNotCompileExtensions = apiOptions.extensions || [];
+		const babelExtensions = apiOptions.babelConfig ? apiOptions.babelConfig.extensions || ['js'] : ['js'];


Oh! Since we're no longer targeting Node.js 4, this could be:

const {extensions: doNotCompileExtensions = []} = apiOptions;

I wouldn't set the default 'js' value here, since it depends on whether apiOptions.babelConfig === null. Instead:

const {extensions: babelExtensions = []} = apiOptions.babelConfig || {}; if (!apiOptions.babelConfig && doNotCompileExtensions.length === 0) { doNotCompileExtensions.push('js'); } if (apiOptions.babelConfig && babelExtensions.length === 0) { babelExtensions.push('js'); }

Some of this could be done through destructuring but it gets rather difficult to understand.

Oh alright I see.

novemberborn · 2018-04-07T17:10:31Z

api.js

+		const babelExtensions = apiOptions.babelConfig ? apiOptions.babelConfig.extensions || ['js'] : ['js'];
+
+		// Combine all extensions possible for testing.
+		const allExts = [...doNotCompileExtensions, ...babelExtensions];


This needs a check to ensure any extension in doNotCompileExtensions is not also listed in babelExtensions.

For this im just using concat then filter to filter out any duplicate entries:

const allExtensions = doNotCompileExtensions .concat(babelExtensions) .filter((ext, i, self) => self.indexOf(ext) === i);

novemberborn · 2018-04-07T17:15:17Z

lib/babel-config.js

+			return false;
+		}
+	}
+	return true;


How about:

return Object.keys(conf).every(key => key === 'extensions' || key === 'testOptions')`

So elegant!

novemberborn · 2018-04-07T17:16:48Z

lib/babel-config.js

 		throw new Error(`${colors.error(figures.cross)} Unexpected Babel configuration for AVA. See ${chalk.underline('https://github.com/avajs/ava/blob/master/docs/recipes/babel.md')} for allowed values.`);
 	}

+	if (conf.extensions === undefined) {
+		conf.extensions = ['js'];


I think it might be easier not to set a default here, but always ensure extensions is an array. Then the code in api.js could be simplified further.

I'm just going to strip away this code, this shouldn't even be needed with the recommendations you gave for api.js.

novemberborn · 2018-04-07T17:17:55Z

lib/babel-config.js

 	}

-	if (!conf || typeof conf !== 'object' || !conf.testOptions || typeof conf.testOptions !== 'object' || Array.isArray(conf.testOptions) || Object.keys(conf).length > 1) {
+	if (!conf || typeof conf !== 'object' || !conf.testOptions || typeof conf.testOptions !== 'object' || Array.isArray(conf.testOptions) || !validateKeys(conf)) {


This needs to check whether extensions, if present, is an array.

It might be worthwhile creating a let valid = true variable and then splitting the checks up into multiple if conditions, each of which could assign false, before throwing the error only if !valid.

Instead of multiple if conditions we could use invalid instead then split them up by doing:

let invalid = false; invalid = !conf || typeof conf !== 'object' || ...; invalid = ... if (invalid) { throw ... }

This should be a bit shorter than a bunch of if statements.

novemberborn · 2018-04-07T17:18:31Z

test/api.js

 const Api = require('../api');

+const test = tap.test;


Why this change? (Same for the other files.)

This was an oops, I was running tap.only to test specific files and forgot to revert this line.

novemberborn · 2018-04-08T16:05:08Z

@Jaden-Giordano if you could merge in (or rebase on) master, that should fix some of the CI issues.

greym0uth · 2018-04-08T16:18:44Z

Yeah will do!

This is an update of a previous commit removing a change that was just being used for testing and shouldn't have been pushed

greym0uth · 2018-04-16T21:58:16Z

So the test would just end up precompiling and checking if the test passed but thats what the test I wrote already does so I just left that the same but added a test to ava files to look for files with a specified extension. Also what is the transformSpy issue in the lint, can I fix this or just leave it? It seems to be the only issue.

novemberborn

You actually need to use transformSpy I think, see the inline comment. We also need to exit with an error if the same extension is defined multiple times.

Now something I feel really bad about — we need a separate compilation path for the "do not compile" extensions. Really what that setting means is "don't apply AVA's Babel presets", except that we still want to compile some other enhancements (the compileEnhancements setting), and currently we do that through Babel. I've left a bit more detail inline, below. Please let me know if you want me to push a commit for that, I realize this PR is starting to drag on a bit.

Finally, would you want to add some documentation for this new feature?

Thanks for your hard work on this @Jaden-Giordano. Again apologies for missing some of this issues before. Happy to push some commits if you'd like.

novemberborn · 2018-04-21T16:24:09Z

api.js

+		}
+
+		// Combine all extensions possible for testing. Removing duplicate extensions.
+		const allExtensions = [...new Set([...doNotCompileExtensions, ...babelExtensions])];


This just needs to ensure there is no overlap between both arrays. You could compare the length of allExtensions to the combined length of doNotCompileExtensions and babelExtensions: it should be equal.

We should probably include the ambiguous extensions in the error message though. Some applications of lodash.difference should do the trick.

Given that we're now throwing an error, could you move this logic into the constructor? Feel free to save allExtensions on the instance.

novemberborn · 2018-04-21T16:26:42Z

test/caching-precompiler.js

-	precompiler.precompileFile(fixture('es2015-source-maps.js'));
-	const options = transformSpy.lastCall.args[1];
+	precompiler.precompileFile(fixture('es2015-source-maps.js'), []);
+	const options = babel.transform.lastCall.args[1];


I suspect you may have had a merge conflict here? Use transformSpy rather than babel.transform in this line.

Oh alright gotcha

novemberborn · 2018-04-21T16:32:22Z

api.js

+										try {
+											const realpath = fs.realpathSync(file);
+											let hash = realpath;
+											if (!doNotCompileExtensions.includes(path.extname(realpath))) {


OK so the tricky thing is we still want to compile enhancements, unless that's also disabled. This currently does use Babel, but it won't apply AVA's regular presets. I think this means instantiating a second precompilation object, and doing a second build of Babel configuration.

I feel bad for not spotting this earlier, and it's a bit tricky to line all this up correctly. Let me know if you want me to push a commit for this instead.

@novemberborn If you could on this one I can handle the rest, work is a bit busy so I don't have alot of time to work on this. Thanks for all the help!

novemberborn

If you could on this one I can handle the rest, work is a bit busy so I don't have a lot of time to work on this. Thanks for all the help

@Jaden-Giordano I'm a bit time constrained as well but will tackle the separate "compile enhancements" code path for the regular extensions 👍

I've left some other comments below.

novemberborn · 2018-04-29T16:10:00Z

api.js

+		this.allExtensions = [...new Set([...doNotCompileExtensions, ...babelExtensions])];
+
+		if (this.allExtensions.length !== doNotCompileExtensions.length + babelExtensions.length) {
+			const duplicates = difference([...doNotCompileExtensions, ...babelExtensions]);


You need to compare two arrays for difference to return anything. But then it actually gives the extensions that are not repeated, so:

const notRepeated = difference(doNotCompileExtensions, babelExtensions); const duplicates = this.allExtensions.filter(ext => !notRepeated.includes(ext));

I suppose this still needs a test?

novemberborn · 2018-04-29T16:11:34Z

readme.md

@@ -262,6 +262,7 @@ To ignore a file or directory, prefix the pattern with an `!` (exclamation mark)
 			"@babel/register"
 		],
 		"babel": {
+			"extensions": ["jsx"]


Missing comma.

novemberborn · 2018-04-29T16:14:40Z

readme.md

@@ -282,8 +283,10 @@ Arguments passed to the CLI will always take precedence over the configuration i
 - `tap`: if `true`, enables the [TAP reporter](#tap-reporter)
 - `snapshotDir`: specifies a fixed location for storing snapshot files. Use this if your snapshots are ending up in the wrong location
 - `compileEnhancements`: if `false`, disables [power-assert](https://github.com/power-assert-js/power-assert) — which otherwise helps provide more descriptive error messages — and detection of improper use of the `t.throws()` assertion
+- `extensions`: Extensions to opt of of precompilation.


This needs some elaboration. How about:

Extensions of test files that are not precompiled using AVA's Babel presets. Note that files are still compiled to enable power-assert and other features, so you may also need to set compileEnhancements to false if your files are not valid JavaScript.

novemberborn · 2018-04-29T16:15:00Z

readme.md

 - `require`: extra modules to require before tests are run. Modules are required in the [worker processes](#process-isolation)
 - `babel`: test file specific Babel options. See our [Babel recipe] for more details
+- `babel.extensions`: Extensions to include in precompilation.


How about:

Extensions of test files that will be precompiled using AVA's Babel presets.

novemberborn · 2018-05-07T16:08:47Z

@Jaden-Giordano I haven't had a chance yet to look into this. I just realized we need some changes with Babel (#1789) so I might try and land that before doing my work here.

Anyway this is super close so once that unblocks we should be good to go.

novemberborn · 2018-05-20T16:41:50Z

@Jaden-Giordano just wanted to say I haven't forgotten about this 😄 #1789 is pretty much done so I'm hoping to land that next weekend. I'll then try and get this in as well before cutting a release.

…abel treatment

novemberborn · 2018-05-28T15:30:21Z

Phew! This is ready now, @Jaden-Giordano please have a look at the commits I just pushed.

I've updated the Babel and ES module recipes. I've fixed the top-level extensions / enhancements-only code path, and we can now handle extensions that have periods in them.

novemberborn · 2018-05-28T16:12:17Z

I don't think profile.js script supports custom extensions, but I'll file a follow-up issue for that.

Support periods in extensions.

greym0uth · 2018-05-30T18:24:48Z

Whoo! This is exciting!

novemberborn requested changes Mar 20, 2018

View reviewed changes

Deterministic precompilation with extension property

b54d2c9

greym0uth force-pushed the comma-delimited-filters branch from 2e68b41 to b54d2c9 Compare March 21, 2018 17:07

greym0uth added 3 commits March 29, 2018 12:03

Better glob handling

4dcfe70

Fixed ava filies

17c6b8c

Fixing precompiler tests

2fe0959

novemberborn requested changes Apr 1, 2018

View reviewed changes

novemberborn changed the title ~~Fixes #631 Allowing ambiguous extensions in test filenames~~ Make test file extensions configurable Apr 7, 2018

novemberborn requested changes Apr 7, 2018

View reviewed changes

Fixing api tests that were failing

90de180

This is an update of a previous commit removing a change that was just being used for testing and shouldn't have been pushed

greym0uth force-pushed the comma-delimited-filters branch from 98c978c to 90de180 Compare April 8, 2018 16:24

greym0uth added 4 commits April 8, 2018 12:42

Deterministic precompilation with extension property

943be8e

Better glob handling

fab1148

Fixed ava filies

95662ff

Fixing precompiler tests

d7a4fa1

greym0uth force-pushed the comma-delimited-filters branch from ffd673b to 226b42b Compare April 16, 2018 21:13

Merge branch 'master' into comma-delimited-filters

e5d9562

novemberborn reviewed Apr 21, 2018

View reviewed changes

greym0uth added 3 commits April 26, 2018 16:40

Fixing merge conflict

e6b835b

Moving logic to constructor and documentation

ed63a31

Merge branch 'master' into comma-delimited-filters

f89e44d

novemberborn reviewed Apr 29, 2018

View reviewed changes

novemberborn mentioned this pull request May 28, 2018

AVA with --experimental-modules #1810

Closed

novemberborn added 9 commits May 28, 2018 12:17

Merge branch 'master' into pr/1746

5de1a80

Fix bad merge

51a8d40

Simplify and extend babel config validation

c44bf36

Refactor extension merging in Api

ec4598c

Fix extension name check

fa58da8

Compile enhancements for test files that do not receive the regular B…

566a743

…abel treatment

More test coverage for finding files

0851499

Test various forms of invalid babel config

51b5072

Test enhancement-only compilation

4f64b57

novemberborn approved these changes May 28, 2018

View reviewed changes

novemberborn added 4 commits May 28, 2018 17:15

Update README

e692f8e

Update Babel recipe

2cea3aa

Update ES modules recipe

17cc02c

Refactor, improve and test extension validation

fd477f1

Support periods in extensions.

novemberborn force-pushed the comma-delimited-filters branch from e04c3c7 to fd477f1 Compare May 28, 2018 16:16

novemberborn merged commit 3533aba into avajs:master May 30, 2018

Make test file extensions configurable #1746

Make test file extensions configurable #1746

Conversation

greym0uth commented Mar 19, 2018 • edited by novemberborn Loading

greym0uth commented Mar 19, 2018

novemberborn left a comment

Choose a reason for hiding this comment

greym0uth commented Mar 20, 2018

greym0uth commented Mar 21, 2018

novemberborn commented Mar 21, 2018

greym0uth commented Mar 21, 2018 • edited Loading

greym0uth commented Mar 21, 2018 • edited Loading

greym0uth commented Mar 25, 2018

novemberborn commented Mar 26, 2018

greym0uth commented Mar 26, 2018

greym0uth commented Mar 31, 2018

Update:

Next steps after fixes:

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

greym0uth commented Apr 3, 2018

novemberborn commented Apr 4, 2018

greym0uth commented Apr 4, 2018

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

novemberborn commented Apr 8, 2018

greym0uth commented Apr 8, 2018

greym0uth commented Apr 16, 2018 • 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

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

novemberborn commented May 7, 2018

novemberborn commented May 20, 2018

novemberborn commented May 28, 2018

novemberborn commented May 28, 2018

greym0uth commented May 30, 2018

greym0uth commented Mar 19, 2018 •

edited by novemberborn

Loading

greym0uth commented Mar 21, 2018 •

edited

Loading

greym0uth commented Mar 21, 2018 •

edited

Loading

greym0uth commented Apr 16, 2018 •

edited

Loading