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

Fixed an issue where espower-babel/guess didn't work when required in gulpfile.js #24

Merged
merged 6 commits into from
Dec 19, 2015
Merged

Fixed an issue where espower-babel/guess didn't work when required in gulpfile.js #24

merged 6 commits into from
Dec 19, 2015

Conversation

nodaguti
Copy link
Contributor

Overview

This PR fixes an issue where process.argv sniffing prevented the require hook from working under certain envs by making the sniffing more strict and accurate.

The issue details

When espower-babel/guess require hook was invoked by a process with an argument containing ':', guess.js failed to require.resolve() and threw an error.
For example, when I executed the following command, it ended with the following error message.

Command line

$ gulp test:e2e

gulpfile.babel.js

import gulp from 'gulp';
import mocha from 'gulp-mocha';
import {} from 'espower-babel/guess';

const paths = {
  e2e: './test/e2e/**/*.spec.js',
};

gulp.task('test:e2e', () =>
  gulp.src(paths.e2e)
    .pipe(mocha({
      timeout: 10000,
    }))
);

Error message

$ gulp test:e2e

[21:46:30] Requiring external module babel-core/register
module.js:340
    throw err;
    ^

Error: Cannot find module 'e2e'
    at Function.Module._resolveFilename (module.js:338:15)
    at Function.require.resolve (module.js:389:19)
    at /Users/***/workspace/e2e-sample/node_modules/espower-babel/guess.js:17:36
    at Array.forEach (native)
    at Object.<anonymous> (/Users/***/workspace/e2e-sample/node_modules/espower-babel/guess.js:10:14)
    at Module._compile (module.js:425:26)
    at Module._extensions..js (module.js:432:10)
    at Object.require.extensions.(anonymous function) [as .js] (/Users/***/workspace/e2e-sample/node_modules/babel-register/lib/node.js:138:7)
    at Module.load (module.js:356:32)
    at Function.Module._load (module.js:313:12)

Improve process.argv sniffing
a34aed3 
Fixed an issue preventing espower-babel/guess require hook from working
when it's invoked by outside of mocha cli, such as gulpfile.js or test/*.js.
azu
azu reviewed Dec 18, 2015
View reviewed changes
guess.js
// <extension>:espower-babel/guess
var args = arg.split(':');
if (args.length <= 1) {
if (!~arg.indexOf(':')) {
Copy link
Member

Choose a reason for hiding this comment

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

It should be arg.indexOf(':') === -1.

@nodaguti
Copy link
Contributor Author

Fixed the mentioned point and added a test.

@nodaguti
Copy link
Contributor Author

The added test has failed due to the timeout, while in my computer it wasn't failed.
Try extending the timeout limit...

azu
azu reviewed Dec 19, 2015
View reviewed changes
gulpfile.js
@@ -0,0 +1,15 @@
/**
Copy link
Member

Choose a reason for hiding this comment

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

Is gulpfile.js required on top(root) directory?
Can you move it to test/issues/24 dir?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, sorry for that, sure.

azu
azu reviewed Dec 19, 2015
View reviewed changes
package.json
@@ -22,7 +22,7 @@
"lib"
],
"scripts": {
"test": "mocha --require './guess' test/**/*.js && mocha --require './test_loader/espower-traceur-loader' test/**/*.js && mocha test/issues/17 --compilers es6:./guess"
"test": "mocha --require './guess' test/**/*.js && mocha --require './test_loader/espower-traceur-loader' test/**/*.js && mocha test/issues/17 --compilers es6:./guess && mocha test/issues/24 --compilers js:./guess --timeout 5000"
Copy link
Member

Choose a reason for hiding this comment

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

--timeout 15000 🆙 ❓

Copy link
Member

Choose a reason for hiding this comment

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

✓ can run a task whose name contains a colon (7445ms)

☑️

@nodaguti
Copy link
Contributor Author

--timeout 15000 is enough! This time I made it, thanks for many advices!
(In my computer, that test is finished within 2000ms...)

azu added a commit that referenced this pull request Dec 19, 2015
@azu
Merge pull request #24 from nodaguti/fix-require-hook
f26895d 
Fixed an issue where espower-babel/guess didn't work when required in gulpfile.js
@azu azu merged commit f26895d into power-assert-js:master Dec 19, 2015
@azu
Copy link
Member

azu commented Dec 19, 2015

@nodaguti OK. Thank for work.
Merged!

@azu
Copy link
Member

azu commented Dec 19, 2015

Release 4.0.1

@azu
Copy link
Member

azu commented Dec 19, 2015

I found that Node.js 0.12 is very slow.
As a result, mocha reach timeout on Node.js 0.12. https://travis-ci.org/power-assert-js/espower-babel/builds/97844354

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@nodaguti @azu