Error refactoring and exit code fixes #2046

na-- · 2021-06-02T16:21:45Z

This PR does a few things:

refactors (and I think simplifies) a bunch of error handling and exit code determination
fixes the bug of k6 returning an exit code of 103 for script errors in VU>0 initialization, while preserving the helpful hints
(breaking change) changes one k6 cloud exit code from 99 to 97, to make it distinct from the already existing 99 exit code for k6 run (the k6 cloud code doesn't always mean that the thresholds have failed)

codecov-commenter · 2021-06-02T16:28:32Z

Codecov Report

Merging #2046 (66443b6) into master (86315c6) will increase coverage by 0.16%.
The diff coverage is 41.25%.

❗ Current head 66443b6 differs from pull request most recent head d8e41ec. Consider uploading reports for the commit d8e41ec to get more accurate results

@@            Coverage Diff             @@
##           master    #2046      +/-   ##
==========================================
+ Coverage   71.72%   71.89%   +0.16%     
==========================================
  Files         179      181       +2     
  Lines       14100    14116      +16     
==========================================
+ Hits        10113    10148      +35     
+ Misses       3349     3337      -12     
+ Partials      638      631       -7

Flag	Coverage Δ
ubuntu	`71.81% <41.25%> (+0.18%)`	⬆️
windows	`71.51% <41.25%> (+0.16%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
cmd/archive.go	`11.53% <0.00%> (-0.23%)`	⬇️
cmd/cloud.go	`4.73% <0.00%> (ø)`
cmd/common.go	`36.36% <ø> (ø)`
cmd/root.go	`8.27% <0.00%> (-0.55%)`	⬇️
cmd/run.go	`13.04% <0.00%> (+0.89%)`	⬆️
js/runner.go	`83.42% <0.00%> (+1.55%)`	⬆️
lib/types/types.go	`89.55% <ø> (ø)`
core/local/local.go	`78.09% <50.00%> (+2.38%)`	⬆️
js/errors.go	`52.38% <52.38%> (ø)`
cmd/config.go	`87.50% <100.00%> (+2.08%)`	⬆️
... and 15 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 86315c6...d8e41ec. Read the comment docs.

mstoykov · 2021-06-03T07:29:34Z

errext/exit_code.go

+	if errors.As(err, &ecerr) {
+		// The given error already has an exit code, do nothing
+		return err
+	}


I think I would never expect this to happen.

Is there a situation where that is a problem?

I use this property in https://github.com/k6io/k6/blob/1d741a854b1b18cd3af99f397d857375a630648c/cmd/run.go#L248-L252

The idea is that, since you're wrapping errors, you generally want to keep the most specific exit code possible. So having the WithExitCode not overwrite an already existing attached error code is very helpful - in the above code, if we already had a an exit code (e.g. a script error) we'll exit with its code (e.g. 107), but if we don't, we'd still attach a the more generic one there.

And, of course, you can always negate that with a custom error type (or another helper), if some other use case requires masking of a previously attached exit code. But this makes sense to me as the default behavior.

I am totally not sold on the "This is already wrapped in error fo the "same type" so I will silently not wrap it for you" idea. IMO w/e code will exit with the code should check whether it was wrapped multiple times and decide whether it should exit with the most inner one or not. Or for the code that is wrapping to check if it has ErrorCode and either wrap it again ... or not.

I am win with WithExitCodeIfNotOneAlready being added

hmm fair enough... I am not sure if I should just rename this to WithExitCodeIfNotOneAlready / WithExitCodeIfNone, since we don't really have a use case for something that always attaches an exit code yet, in most places it either doesn't matter (root errors deep in the code that first attach an exit code) or we don't want to overwrite it

alternatively, I can rip it out and only check for existing exit codes in a few places, I'll see how that looks like

Is this ok? d8e41ec

imiric

This is cleaner than before, and I like the With* and interfaces approach, but I'd prefer it if errext wasn't a top-level package, but maybe cmd/errors. There's really no reason an internal package should ever import it, and having it globally available suggests otherwise.

errext/exitcodes/codes.go

cmd/config.go

js/runner.go

js/timeout_error.go

na-- · 2021-06-07T07:23:28Z

@imiric I tried to shuffle things around, but adding any more things to cmd just made things messier and more error-prone. Even just moving the exit codes in there made things more difficult to test... 😞 I may be missing some clever trick on how to have them tidy, so do you want to make a PoC PR on top of this one or master with how you think this should be handled?

imiric · 2021-06-07T07:42:40Z

@na-- I doubt I'll be able to make progress if you haven't 😄 I'll take a look, but let's not delay merging this because of it.

imiric

@na-- I did some trivial changes in #2050, but otherwise this LGTM.

js/timeout_error.go

mstoykov

I am not going to "request changes", but I am still not on board with the whoever returns errors to say what the exit code should be.

I have left some other comments which mostly resolve around problems that I think raise from that.

mstoykov · 2021-06-08T11:17:50Z

errext/exit_code.go

+	if errors.As(err, &ecerr) {
+		// The given error already has an exit code, do nothing
+		return err
+	}


I am totally not sold on the "This is already wrapped in error fo the "same type" so I will silently not wrap it for you" idea. IMO w/e code will exit with the code should check whether it was wrapped multiple times and decide whether it should exit with the most inner one or not. Or for the code that is wrapping to check if it has ErrorCode and either wrap it again ... or not.

I am win with WithExitCodeIfNotOneAlready being added

errext/hint.go

mstoykov · 2021-06-08T11:22:24Z

js/errors.go

+	inner *goja.Exception
+}
+
+var _ errext.Exception = &scriptException{}


You should probably check for the HasHint and HasErrorCode while you are at it.

although to be honest I am not certain if I wouldn't have preferred there to be newScriptExceiption() which calls

WithHint(WithExitCode(scriptException{inner: ex}...))

On the other hand my biggest problem is that errext.ScriptException implementations can have different exitcodes ... which again is the whole problem with the exit codes being codified by whatever returns them instead of what will exit with them ...

On the other hand my biggest problem is that errext.ScriptException implementations can have different exitcodes

Well, that's not a bug, it's a feature. errext.ScriptException means the error has a stacktrace, nothing less and nothing more. Exit codes are orthogonal to this. Theoretically, we might want to treat a script exception in handleSummary(), for example, with a different exit code 🤷‍♂️

Decided not to wrap this in WithHint(WithExitCode()), it didn't really improve things in terms of readability or clarity

cmd/root.go

na-- added the breaking change for PRs that need to be mentioned in the breaking changes section of the release notes label Jun 2, 2021

na-- requested review from mstoykov and imiric June 2, 2021 16:21

mstoykov reviewed Jun 3, 2021

View reviewed changes

imiric reviewed Jun 3, 2021

View reviewed changes

errext/exitcodes/codes.go Outdated Show resolved Hide resolved

cmd/config.go Outdated Show resolved Hide resolved

js/runner.go Show resolved Hide resolved

js/timeout_error.go Outdated Show resolved Hide resolved

js/timeout_error.go Show resolved Hide resolved

na-- mentioned this pull request Jun 7, 2021

Improve UX of native Go errors #2049

Closed

na-- force-pushed the error-refactoring-exit-codes branch from 1d741a8 to 8771485 Compare June 7, 2021 07:18

na-- changed the title ~~[WIP] Error refactoring and exit code fixes~~ Error refactoring and exit code fixes Jun 7, 2021

na-- marked this pull request as ready for review June 7, 2021 07:18

na-- requested review from mstoykov and imiric June 7, 2021 07:19

na-- mentioned this pull request Jun 7, 2021

Treat errors from Throw() as plain goja values #1775

Merged

imiric reviewed Jun 7, 2021

View reviewed changes

js/timeout_error.go Show resolved Hide resolved

imiric previously approved these changes Jun 7, 2021

View reviewed changes

mstoykov reviewed Jun 8, 2021

View reviewed changes

na-- added 3 commits June 8, 2021 15:25

Add an errext package with error types, helpers and exitcodes

fec3666

Transition most user-facing errors to use errext

231356f

Fix bug where VU init exceptions weren't properly detected

b21b904

na-- dismissed imiric’s stale review via b21b904 June 8, 2021 12:26

na-- force-pushed the error-refactoring-exit-codes branch from 0c4fbb1 to b21b904 Compare June 8, 2021 12:26

Improve errext helper names and behaviors

d8e41ec

na-- requested review from mstoykov and imiric June 8, 2021 12:28

mstoykov reviewed Jun 8, 2021

View reviewed changes

cmd/root.go Outdated Show resolved Hide resolved

Simplify cmd exit log building

a7b85eb

na-- force-pushed the error-refactoring-exit-codes branch from bbd2541 to a7b85eb Compare June 8, 2021 12:41

imiric approved these changes Jun 8, 2021

View reviewed changes

na-- merged commit 0e1f441 into master Jun 8, 2021

na-- deleted the error-refactoring-exit-codes branch June 8, 2021 15:19

na-- added this to the v0.33.0 milestone Jun 8, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Error refactoring and exit code fixes #2046

Error refactoring and exit code fixes #2046

na-- commented Jun 2, 2021 •

edited

Loading

codecov-commenter commented Jun 2, 2021 •

edited

Loading

mstoykov Jun 3, 2021

na-- Jun 3, 2021

na-- Jun 3, 2021

mstoykov Jun 8, 2021

na-- Jun 8, 2021

na-- Jun 8, 2021

imiric left a comment

na-- commented Jun 7, 2021

imiric commented Jun 7, 2021

imiric left a comment •

edited

Loading

mstoykov left a comment

mstoykov Jun 8, 2021

mstoykov Jun 8, 2021

mstoykov Jun 8, 2021

na-- Jun 8, 2021

na-- Jun 8, 2021

Error refactoring and exit code fixes #2046

Error refactoring and exit code fixes #2046

Conversation

na-- commented Jun 2, 2021 • edited Loading

codecov-commenter commented Jun 2, 2021 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

imiric left a comment

Choose a reason for hiding this comment

na-- commented Jun 7, 2021

imiric commented Jun 7, 2021

imiric left a comment • edited Loading

Choose a reason for hiding this comment

mstoykov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

na-- commented Jun 2, 2021 •

edited

Loading

codecov-commenter commented Jun 2, 2021 •

edited

Loading

imiric left a comment •

edited

Loading