Skip to content
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

Jump to bottom

Precompile helpers #1078

Merged
merged 2 commits into from
Jan 7, 2017
Merged

Precompile helpers #1078

merged 2 commits into from
Jan 7, 2017

Conversation

vadimdemedes
Copy link
Contributor

@vadimdemedes vadimdemedes commented Oct 15, 2016 •
edited
Loading

Fixes #720 and potentially #1049.

Lack of this feature has been bugging me for a long time, glad I've finally found a time to fix it. It's still WIP, need to:

  • Submit a PR to ava-files to support findHelpers() (currently named as findTestDependencies()).
  • Fix tests.
  • Write new tests.

As for #1049 (external macros are not precompiled), we could add a special glob for that, like **/macros/** to precompile those files as well.

@novemberborn
Copy link
Member

Good start! This compiles the helpers as if they were test files, right? That seems correct to me.

Let's see where that gets us before doing anything else for macros.

@sindresorhus sindresorhus force-pushed the master branch 2 times, most recently from 4bc021e to 476c653 Compare October 18, 2016 03:23
@vadimdemedes
Copy link
Contributor Author

This compiles the helpers as if they were test files, right? That seems correct to me.

Yep, exactly.

Let's see where that gets us before doing anything else for macros.

Yes for sure, that is unrelated to this PR.

sindresorhus
sindresorhus reviewed Oct 22, 2016
View reviewed changes
api.js

return new AvaFiles({cwd: this.options.resolveTestsFrom})
.findTestDependencies()
.map(function (file) { // eslint-disable-line array-callback-return
Copy link
Member

Choose a reason for hiding this comment

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

Why use .map if you're not actually going to map anything?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was using it as a way to async iterate the items. Is there another bluebird method that does that?

@vadimdemedes vadimdemedes force-pushed the precompile-helpers-fixtures branch from 600f1d3 to b5ace1c Compare October 23, 2016 09:16
@vadimdemedes vadimdemedes mentioned this pull request Oct 23, 2016
Merged
@vadimdemedes vadimdemedes force-pushed the precompile-helpers-fixtures branch 3 times, most recently from 9303025 to f0f25b5 Compare October 23, 2016 12:44
novemberborn
novemberborn reviewed Oct 23, 2016
View reviewed changes
Copy link
Member

@novemberborn novemberborn left a comment

Choose a reason for hiding this comment

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

Do we foresee people wanting to disable this behavior?

test/api.js
@@ -705,9 +705,16 @@ function generateTests(prefix, apiCreator) {
test(prefix + 'caching is enabled by default', function (t) {
t.plan(3);
rimraf.sync(path.join(__dirname, 'fixture/caching/node_modules'));

var prevCwd = process.cwd();
process.chdir(path.join(__dirname, 'fixture/caching'));
Copy link
Member

Choose a reason for hiding this comment

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

Why this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tests are being run from the root, so AVA finds and precompiles helpers and fixtures from the test folder (e.g. test/fixture/ignored-dirs/helpers/test.js).

That's why I change the cwd to make it look like the tests are being executed from that dir. I think it's actually what each test should do.

Copy link
Member

Choose a reason for hiding this comment

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

The pkgDir and resolveTestsFrom directories can be passed to Api though. That's how its meant to be used, implicit defaults aside. We shouldn't need to change the working directory of the tap process.

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 for the tip!

@novemberborn
Copy link
Member

Do we foresee people wanting to disable this behavior?

Specifically transpiling fixtures seems fraught.

@vadimdemedes
Copy link
Contributor Author

Specifically transpiling fixtures seems fraught.

Could you elaborate on this? I don't quite understand the reasoning.

@novemberborn
Copy link
Member

Helpers should contain logic to make it easier to write test. Fixtures contain all sorts of files that relate to your test. If you're testing a Babel plugin you may write .js fixtures that you're using to exercise your tests. Those should not be transpiled.

@sindresorhus
Copy link
Member

@vdemedes What's your use-case for transpiling fixtures? I'm leaning towards @novemberborn's argument.

@vadimdemedes
Copy link
Contributor Author

My thinking was that everything that resides under test/ belongs to AVA, so it should take care of those files and provide its features (power-assert, ES6, etc) to them as well.

However, @novemberborn's reasoning about fixtures relation to tests does make sense to me now. I will correct this PR and the one in ava-files as well.

@vadimdemedes vadimdemedes force-pushed the precompile-helpers-fixtures branch from f0f25b5 to fe3eec1 Compare November 6, 2016 11:58
@sindresorhus sindresorhus force-pushed the master branch 4 times, most recently from 8610723 to 4acb78f Compare November 22, 2016 17:46
@vadimdemedes vadimdemedes force-pushed the precompile-helpers-fixtures branch from fe3eec1 to 187b0a6 Compare December 12, 2016 20:37
@vadimdemedes vadimdemedes changed the title [WIP] Precompile helpers and fixtures Precompile helpers and fixtures Dec 12, 2016
@vadimdemedes vadimdemedes changed the title Precompile helpers and fixtures Precompile helpers Dec 12, 2016
@Snugug
Copy link

Snugug commented Dec 16, 2016

w/r/t #1049, I've just installed this branch and ran against an external macro, and it did not appear to resolve the issue that powerAssert is not applied to those tests

@vadimdemedes
Copy link
Contributor Author

@Snugug only files with names starting with _ and located in helpers/ folders get transpiled in this PR. In the issue you mentioned the files are located in macros/ folder, right?

@vadimdemedes vadimdemedes force-pushed the precompile-helpers-fixtures branch from 187b0a6 to 3e8523a Compare December 27, 2016 20:51
@sindresorhus
Copy link
Member

Code looks good to me, but needs to be added to the readme.

@vadimdemedes vadimdemedes force-pushed the precompile-helpers-fixtures branch from 3e8523a to c3fe5ec Compare January 5, 2017 13:39
@vadimdemedes
Copy link
Contributor Author

Added a small note to readme: https://github.com/avajs/ava/pull/1078/files#diff-0730bb7c2e8f9ea2438b52e419dd86c9R738.

@sindresorhus sindresorhus merged commit 410cb8d into master Jan 7, 2017
@sindresorhus sindresorhus deleted the precompile-helpers-fixtures branch January 7, 2017 08:44
@sindresorhus
Copy link
Member

sindresorhus commented Jan 7, 2017 •
edited
Loading

Yay! This one is going to be super useful 💃

Thanks for finishing this up Vadim 👌

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sindresorhus sindresorhus sindresorhus left review comments

@novemberborn novemberborn novemberborn left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Transpile helper files
4 participants
@vadimdemedes @novemberborn @sindresorhus @Snugug