Skip to content

Conversation

@JPeer264
Copy link
Contributor

I updated some tests to improve the coverage.


function cb(err) {
expect(function() {
throw err;
Copy link
Member

Choose a reason for hiding this comment

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

why do we need to throw this error? I think you can test the message with match or something.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The thing is if I test it with expect(err).toMatch(/Missing completion type/) I can type in any error message, it will still match the thrown error: expect(err).toMatch(/Any error/) would still pass.

If I throw err inside the function I can catch the error message with .toThrow.

Copy link
Member

Choose a reason for hiding this comment

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

Why would that match? Maybe you are using the wrong assertion. Also, you probably need to match err.message

Copy link
Contributor Author

Choose a reason for hiding this comment

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

true that! I updated now the test with .toMatch

@phated
Copy link
Member

phated commented Jan 11, 2017

Looks to have failed on Windows (node 0.10) - please take a look at that.

@JPeer264
Copy link
Contributor Author

Just saw it, I will take a look at it.

@JPeer264
Copy link
Contributor Author

You think it is legit to write something like

if (process.version.slice(0, 5) === 'v0.10' && process.platform === 'win32')

@phated
Copy link
Member

phated commented Jan 11, 2017

@JPeer264 where is "Command failed" coming from? Is that the Windows platform or from within gulp-cli?

@JPeer264
Copy link
Contributor Author

JPeer264 commented Jan 11, 2017

@phated very interesting. I started my VM with Windows 10 and it turned out that Node gives an uncaught expection with code: 8. It seems it cannot handle throw correctly

@JPeer264
Copy link
Contributor Author

JPeer264 commented Jan 11, 2017

@phated just a guess where it comes from. In gulp-test-tools@gulp-runner.js everything on windows below v0.12 will execute the command directly. Because it throws an error, the shell might not understand the error and it will get the uncaught error.

@phated
Copy link
Member

phated commented Jan 11, 2017

pinging @sttk to fix that inside gulp-test-tools

@sttk
Copy link
Contributor

sttk commented Jan 14, 2017

@JPeer264 On v0.10, gulpRunner in gulp-test-tools redirects stdout and stderr to files because there were cases that stdout/stderr of previous test case is mixed. Therefore the content of stderr was not contained in err.message.

Since the callback function of gulpRunner#run receives arguments: err, stdout and stderr, you can check stderr by the code like:

runner().gulp('--completion').run(cb);
function cb(err, stdout, stderr) {
  expect(stderr).toMatch('Missing completion type');
}

In addition, you can show the contents of err, stdout, stderr by passing verbose flag to gulpRunner, like: (should be false when committing though)

runner({ verbose: true }).gulp('--completion').run(cb);
function cb(err, stdout, stderr) {
  expect(stderr).toMatch('Missing completion type');
}
$ node_modules\.bin\mocha test\completion.js

  ...

---- command
cd C:\\Users\\sttk\\Works\\gulp-cli &
node C:\\Users\\sttk\\Works\\gulp-cli\\bin\\gulp.js --completion > C:\Users\sttk\AppData\Local\Temp\gulp-test-1484316642968986\result.out 2> C:\Users\sttk\AppData\Local\Temp\gulp-test-1484316642968986\result.err
---- error
{ [Error: Command failed: ] killed: false, code: 8, signal: null }
---- stdout

---- stderr

C:\Users\sttk\Works\gulp-cli\lib\shared\completion.js:8
    throw new Error('Missing completion type');
          ^
Error: Missing completion type
    at Liftoff.module.exports [as completions] (C:\Users\sttk\Works\gulp-cli\l
ib\shared\completion.js:8:11)
    at Liftoff.launch (C:\Users\sttk\Works\gulp-cli\node_modules\liftoff\index.js:182:17)
    at run (C:\Users\sttk\Works\gulp-cli\index.js:87:7)
    at Object.<anonymous> (C:\Users\sttk\Works\gulp-cli\bin\gulp.js:5:15)
    at Module._compile (module.js:456:26)
    at Object.Module._extensions..js (module.js:474:10)
    at Module.load (module.js:356:32)
    at Function.Module._load (module.js:312:12)
    at Function.Module.runMain (module.js:497:10)
    at startup (node.js:119:16)

----.

@JPeer264
Copy link
Contributor Author

@sttk Thanks for this nice explanation!

@phated phated merged commit f3b4d7e into gulpjs:master Jan 17, 2017
@phated
Copy link
Member

phated commented Jan 17, 2017

Thanks @JPeer264!

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants