Don't fail on stderr output, but print the output to stderr #110

Tapppi · 2016-09-22T17:28:38Z

For #91, this would stop standard-version from failing fast when git or hooks or something prints to stderr, but prints it out. Added test case it for as well. Also, errors are now printed to stderr/console.error instead of stdout/console.log.

/CC @nexdrew @duclet @e-cloud @stevemao @bcoe

coveralls · 2016-09-22T17:32:11Z

Coverage decreased (-0.3%) to 94.805% when pulling 82cabab on 91-no-fail-fast into 2ce4160 on master.

Tapppi · 2016-09-22T17:36:14Z

I suspect coverage decrease is because the actual amount of code went down, since the git tags error handling got rolled into handleExecError? Can't figure out another reason.

stevemao · 2016-09-22T23:45:29Z

index.js

@@ -108,6 +108,17 @@ function outputChangelog (argv, cb) {
  })
 }

+function handleExecError (err, stderr) {


I thought we should handle err and stderr differently?
the problem is with stderr shouldn't terminate the program. But for err we keep the same behaviour. No?

I think I misread your code. It LGTM now.

stevemao

Is it possible to test exit code for all different scenarios?

stevemao · 2016-09-22T23:50:37Z

I suspect coverage decrease is because the actual amount of code went down, since the git tags error handling got rolled into handleExecError? Can't figure out another reason.

You can check out the coverage output and see why

e-cloud · 2016-09-23T01:34:55Z

index.js

+  // If exec returns an error or content in stderr, log it and exit with return code 1
+  var errMessage = stderr || (err && err.message)
+  if (errMessage) {
+    console.error(chalk.red(errMessage))


i think when err is null, better to use console.warn to output the error message. Red message means real error.
Since console.error/warn's output will have its own color, should it continue to use chalk to control the color?

Since console.error/warn's output will have its own color

I don't think so

The console.warn() function is an alias for console.error().
https://nodejs.org/api/console.html#console_console_warn_data

I think we should print to stderr (console.error), but how about yellow color for when the exit code was 0?

Since console.error/warn's output will have its own color, should it continue to use chalk to control the color?

AFAIK chalk should handle coloring the output correctly regardless of the stream it will be printed to.

sorry for my poor knowledge, yellow would be great!

Done in 458da35

Tapppi · 2016-09-23T11:09:55Z

@stevemao:

Is it possible to test exit code for all different scenarios?

What do you mean by this? What scenarios should we be checking for? One thing that is missing, is rolling back changes made by standard-version when the whole process doesn't go through, but I think that's another PR as it's a bigger change.

You can check out the coverage output and see why

Thanks! Yeah, the missed line count stayed the same at 4, but overall line count went down. Should actually find out what is not being tested.. I'll get to it soon, as I'll have some time.

coveralls · 2016-09-23T15:21:00Z

Coverage decreased (-0.3%) to 94.805% when pulling 458da35 on 91-no-fail-fast into 2ce4160 on master.

stevemao · 2016-09-24T00:05:54Z

Is it possible to test exit code for all different scenarios?

~~looks like we only test result.code.should.equal(0), but not 1. As long as we cover all scenarios we are fine :)~~

Nice work! @Tapppi

e-cloud

code logic could be more clear

e-cloud · 2016-09-24T04:52:25Z

index.js

  var errMessage = stderr || (err && err.message)
  if (errMessage) {
-    console.error(chalk.red(errMessage))
+    console.error(err ? chalk.red(errMessage) : chalk.yellow(errMessage))
    if (err) {
      process.exit(1)
    }


@Tapppi , i think it would be better to be:

if (err) { console.error(chalk.red(stderr || err.message)) process.exit(1) } else if(stderr) { console.warn(chalk.yellow(stderr)) }

the prior stderr in console.error could be dropped.

Good idea, changed in 6338f32

coveralls · 2016-09-24T17:17:34Z

Coverage decreased (-0.3%) to 94.805% when pulling 6cf3cbd on 91-no-fail-fast into 2ce4160 on master.

coveralls · 2016-09-24T17:22:24Z

Coverage decreased (-0.3%) to 94.805% when pulling 6338f32 on 91-no-fail-fast into 2ce4160 on master.

stevemao

LGTM

bcoe · 2016-09-25T00:33:13Z

@Tapppi looks good to me land it.

…#110) * test: add test for not exiting on stderr content and match stderr output * feat: don't fail fast on stderr content, and print stderr, closes #91 * fix: print warnings in yellow instead of red * style: simplify handleExecError if logic

e-cloud · 2016-09-26T02:36:45Z

when will this fix release? coz now it's some kind of blocking bug.

e-cloud · 2016-09-26T02:39:24Z

a beta release is welcomed too.

stevemao · 2016-09-26T23:41:06Z

@bcoe could you publish this?

Tapppi added 2 commits September 22, 2016 19:56

test: add test for not exiting on stderr content and match stderr output

8f0623b

feat: don't fail fast on stderr content, and print stderr, closes #91

82cabab

stevemao reviewed Sep 22, 2016

View reviewed changes

stevemao requested changes Sep 22, 2016

View reviewed changes

e-cloud reviewed Sep 23, 2016

View reviewed changes

fix: print warnings in yellow instead of red

458da35

e-cloud suggested changes Sep 24, 2016

View reviewed changes

style: simplify handleExecError if logic

6338f32

Tapppi force-pushed the 91-no-fail-fast branch from 6cf3cbd to 6338f32 Compare September 24, 2016 17:16

stevemao approved these changes Sep 24, 2016

View reviewed changes

stevemao merged commit f9984d0 into master Sep 25, 2016

stevemao deleted the 91-no-fail-fast branch September 25, 2016 00:46

scottnonnenberg mentioned this pull request Jan 20, 2017

Replace deprecated ghooks with husky scottnonnenberg/eslint-config-thehelp#5

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't fail on stderr output, but print the output to stderr #110

Don't fail on stderr output, but print the output to stderr #110

Tapppi commented Sep 22, 2016

coveralls commented Sep 22, 2016 •

edited

Loading

Tapppi commented Sep 22, 2016

stevemao Sep 22, 2016

stevemao Sep 22, 2016

stevemao left a comment

stevemao commented Sep 22, 2016

e-cloud Sep 23, 2016

stevemao Sep 23, 2016

Tapppi Sep 23, 2016

Tapppi Sep 23, 2016 •

edited

Loading

e-cloud Sep 23, 2016

Tapppi Sep 23, 2016

Tapppi commented Sep 23, 2016

coveralls commented Sep 23, 2016 •

edited

Loading

stevemao commented Sep 24, 2016 •

edited

Loading

e-cloud left a comment

e-cloud Sep 24, 2016

Tapppi Sep 24, 2016

coveralls commented Sep 24, 2016 •

edited

Loading

coveralls commented Sep 24, 2016 •

edited

Loading

stevemao left a comment

bcoe commented Sep 25, 2016

e-cloud commented Sep 26, 2016

e-cloud commented Sep 26, 2016

stevemao commented Sep 26, 2016

Don't fail on stderr output, but print the output to stderr #110

Don't fail on stderr output, but print the output to stderr #110

Conversation

Tapppi commented Sep 22, 2016

coveralls commented Sep 22, 2016 • edited Loading

Tapppi commented Sep 22, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevemao left a comment

Choose a reason for hiding this comment

stevemao commented Sep 22, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tapppi Sep 23, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tapppi commented Sep 23, 2016

coveralls commented Sep 23, 2016 • edited Loading

stevemao commented Sep 24, 2016 • edited Loading

e-cloud left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Sep 24, 2016 • edited Loading

coveralls commented Sep 24, 2016 • edited Loading

stevemao left a comment

Choose a reason for hiding this comment

bcoe commented Sep 25, 2016

e-cloud commented Sep 26, 2016

e-cloud commented Sep 26, 2016

stevemao commented Sep 26, 2016

coveralls commented Sep 22, 2016 •

edited

Loading

Tapppi Sep 23, 2016 •

edited

Loading

coveralls commented Sep 23, 2016 •

edited

Loading

stevemao commented Sep 24, 2016 •

edited

Loading

coveralls commented Sep 24, 2016 •

edited

Loading

coveralls commented Sep 24, 2016 •

edited

Loading