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.

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

Allow test files to have different extensions with transform #324

Merged
merged 5 commits into from Mar 10, 2015

Conversation

@ldesplat
Copy link
Contributor

ldesplat commented Mar 6, 2015

  1. Since we specify transform, our tests themselves could be transformed.
  2. Document a pitfall, which is to not transform too many files so the user has to check for that in transform method.
…ransform. Document potential pitfall
README.md Outdated
return result.code;
}}
// Make sure to only transform your code or the dependencies you want
if (filename.indexOf('node_modules') <= -1) {

This comment has been minimized.

Copy link
@geek

geek Mar 6, 2015

Member

I think this can be ===

lib/cli.js Outdated
@@ -12,7 +12,9 @@ var Utils = require('./utils');

// Declare internals

var internals = {};
var internals = {
pattern: new RegExp('.(js)$')

This comment has been minimized.

Copy link
@geek

geek Mar 6, 2015

Member

generally we do /.(js)$/

@geek geek added the feature label Mar 6, 2015
@geek geek added this to the 5.5.0 milestone Mar 6, 2015
@geek geek self-assigned this Mar 6, 2015
lib/cli.js Outdated
options.transform = transform;

var includes = 'js|' + transform.map(internals.mapTransform).join('|');

This comment has been minimized.

Copy link
@gergoerdosi

gergoerdosi Mar 6, 2015

Contributor

This will create a wrong regex ( .(js|)$ ) if the transform module exports an empty array. Also, the dot should be escaped, below and above too.

…s not empty before building a new regex.
@ldesplat

This comment has been minimized.

Copy link
Contributor Author

ldesplat commented Mar 8, 2015

Thank you so much @gergoerdosi for spotting those issues.

I have fixed the code review issues and file extensions can now include some crazy stuff :). Tested a whole bunch of special characters on Mac, quite interesting!

@geek

This comment has been minimized.

Copy link
Member

geek commented Mar 9, 2015

@ldesplat do we need any new tests for any of the edge cases you were considering with this change?

@ldesplat

This comment has been minimized.

Copy link
Contributor Author

ldesplat commented Mar 10, 2015

don't merge

@ldesplat

This comment has been minimized.

Copy link
Contributor Author

ldesplat commented Mar 10, 2015

Ok, I am sorry. All should be well now.

Thanks for telling me to add tests.
There was a bug where applying transforms with the no code coverage path was different than the coverage path. They are now using the exact same code and it is consistent behavior. This is covered in the 2 new tests.

I had to resort to way too many PRs to add this feature in. Next time I'll hold on a bit longer no matter my enthusiasm! Thanks for all your support.

@AdriVanHoudt

This comment has been minimized.

Copy link
Contributor

AdriVanHoudt commented Mar 10, 2015

@ldesplat thanks for all the effort to add this feature!

geek added a commit that referenced this pull request Mar 10, 2015
Allow test files to have different extensions with transform
@geek geek merged commit 42e983e into hapijs:master Mar 10, 2015
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.