Add JS function to abort test #2093

imiric · 2021-07-13T13:41:18Z

These are the changes from #1920, squashed and rebased on master, while changing the function from k6.abortTest() to the k6/execution module as test.abort(). Additionally a new ExecutionStatusInterrupted was added, a separate exit code (108), and a few other fixes and minor changes. See the commits for details.

Closes #1001

codecov-commenter · 2021-07-13T13:49:35Z

Codecov Report

Merging #2093 (87b85d6) into master (04c8b1f) will decrease coverage by 0.09%.
The diff coverage is 55.42%.

@@            Coverage Diff             @@
##           master    #2093      +/-   ##
==========================================
- Coverage   72.11%   72.01%   -0.10%     
==========================================
  Files         180      181       +1     
  Lines       14249    14319      +70     
==========================================
+ Hits        10275    10312      +37     
- Misses       3348     3375      +27     
- Partials      626      632       +6

Flag	Coverage Δ
ubuntu	`71.94% <55.42%> (-0.10%)`	⬇️
windows	`71.62% <55.42%> (-0.10%)`	⬇️

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

Impacted Files	Coverage Δ
cmd/run.go	`12.39% <0.00%> (-0.65%)`	⬇️
js/modules/modules.go	`81.81% <ø> (ø)`
lib/executor/helpers.go	`86.40% <38.88%> (-10.07%)`	⬇️
js/common/interrupt_error.go	`40.00% <40.00%> (ø)`
core/local/local.go	`77.57% <50.00%> (-0.63%)`	⬇️
js/runner.go	`82.15% <50.00%> (+0.28%)`	⬆️
js/initcontext.go	`86.79% <71.42%> (-1.09%)`	⬇️
js/modules/k6/metrics/metrics.go	`86.53% <93.10%> (+4.72%)`	⬆️
core/engine.go	`85.28% <0.00%> (-0.87%)`	⬇️
... and 1 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 04c8b1f...87b85d6. Read the comment docs.

cmd/run.go

imiric · 2021-07-13T15:25:54Z

Lint errors are from #2073, so ignore :)

And there are currently no tests for this, since they were all at the JS level and were moved to the extension (mostly). Should we add some lower level tests here?

js/common/interrupt_error.go

mstoykov

I didn't understand why you need to use my WIP PRs to test it?

Apart from that and the error comment it seems okay to me

imiric · 2021-07-22T13:08:12Z

I didn't understand why you need to use my WIP PRs to test it?

Because exec.test.abort() is based on grafana/xk6-execution#2, which depends on #2073, so I needed both your changes and these core changes to test the extension. Your commits just don't need to be merged here, so I'll remove them once this is approved, and then you can rebase on master and I'll point the extension PR to your branch instead.

mstoykov

LGTM apart from the possible change to showing the summary

cmd/run.go

na--

LGTM in principle, but I didn't review this very deeply, mostly relied on my memories from reviewing the old PR 😅

Resolves #2093 (comment)

mstoykov · 2021-08-13T10:12:19Z

cmd/run.go

+			if runErr != nil {
+				if common.IsInterruptError(runErr) {
+					return errext.WithExitCodeIfNone(runErr, exitcodes.ScriptException)
+				}
+				return errext.WithExitCodeIfNone(runErr, exitcodes.GenericEngine)


this also changes that we will show the summary on non InterruptError which probably is not what we want

No? I did that intentionally as I don't see why we shouldn't show the summary in all cases.

I'll leave this as is, but you guys feel free to change it before merging.

I see some merit to showing the summary in more cases but I think this is kind of a bigger and different change so I would prefer to have it outside of this PR

cc @na--

I removed this in the latest force push, so now only interrupt errors should show the summary.

Resolves #2093 (comment)

imiric · 2021-12-17T09:22:58Z

@codebien I wanted to keep the original commit mostly intact, as it was authored by gernest. The changes on top of it are mostly atomic new features and shouldn't be squashed, but I can rebase it on current master and clean it up a bit. I'll update the PR description as well.

Co-authored-by: Ivan Mirić <[email protected]> Closes #1001 This adds abortTest() helper function to the k6 module. This function when called inside a script it will - stop the whole test run and k6 will exit with 107 status code - stops immediately the VU that called it and no more iterations are started - make sure the teardown() is called - the engine run status is 7 (RunStatusAbortedScriptError) `(*goja.Runtime).Interrupt` is used for halting script execution and capturing stack traces for better error message of what is happening with the script. We introduce InterruptError which is used with `(*goja.Runtime).Interrupt` to identify interrupts emitted by abortTest(). This way we use special handling of this type. Example script is ```js import { abortTest, sleep } from 'k6'; export default function () { // We abort the test on second iteration if (__ITER == 1) { abortTest(); } sleep(1); } export function teardown() { console.log('This function will be called even when we abort script'); } ``` abortTest() can be called in both default and setup functions, however you can't use it in the init context The following script will fail with the error ``` ERRO[0000] Using abortTest() in the init context is not supported at (...path to the script )init.js:13:43(34) ``` ```js import { abortTest } from 'k6'; abortTest(); export function setup() { } export default function () { // ... some test logic ... console.log('mayday, mayday'); } ``` You can customize the reason for abortTest() by passing values to the function ```js abortTest("Exceeded expectations"); ``` Will emit `"Exceeded expectations"` on the logs

Resolves #2093 (comment)

This is needed to be able to read the test run status via the API, e.g. in the Cloud.

Resolves #2093 (comment)

This ensures we exit with code 108 (ScriptAborted) in all places where it might be called from.

See #2093 (review)

codebien

Should we add for this?

imiric · 2021-12-17T13:09:22Z

5 months later and almost 9 months since #1920, we're finally merging this! 😅 It's a Christmas miracle! 😄🎄

Thanks for the reviews everyone! I'll try to add some documentation for this next week.

Actually, spoke too soon... The CLA not passing is blocking it 🤦‍♂️

Anyone know how to retrigger it? Or should we just merge this manually?

Ah, it won't pass because gernest hasn't signed the new CLA. Since he did sign the previous one and the new one just has branding changes, should we manually merge this or ping him here?

na-- · 2021-12-17T13:38:19Z

He signed the old CLA in #1920 (comment), which is basically identical to the current one for k6 (the new one only expands to cover other xk6 projects), so it's fine to merge as it is.

Resolves #2093 (comment)

See #2093 (review)

jackgoodacre-crypto · 2022-01-13T16:32:17Z

Hey all,

Have found my way onto this PR as a result of looking for the ability to abort k6 execution with non-zero exit code. I appreciate the efforts required to develop these changes, so thank you for building on top of suggestions/requests coming from users.

I was just wondering, although this functionality is merged to master I don't believe it is available in an official version yet right? If not, are there currently any estimations as to when this may occur?

Thanks again

UPDATE 20220128: I see this is now released under v0.36.0. Tyvm

Resolves grafana/k6#2093 (comment)

See grafana/k6#2093 (review)

imiric requested review from mstoykov, codebien and na-- July 13, 2021 13:41

imiric mentioned this pull request Jul 13, 2021

Add test.abort() function grafana/xk6-execution#3

Closed

imiric commented Jul 13, 2021

View reviewed changes

cmd/run.go Outdated Show resolved Hide resolved

mstoykov reviewed Jul 22, 2021

View reviewed changes

js/common/interrupt_error.go Outdated Show resolved Hide resolved

mstoykov reviewed Jul 22, 2021

View reviewed changes

imiric force-pushed the feat/1001-abort-test branch from 87b85d6 to 913c0bf Compare July 23, 2021 10:39

mstoykov mentioned this pull request Aug 5, 2021

common.bind replacement #2108

Merged

imiric force-pushed the feat/1001-abort-test branch 2 times, most recently from b899f9e to 314e25c Compare August 6, 2021 15:05

imiric requested a review from mstoykov August 12, 2021 08:42

imiric changed the base branch from master to feat/common.BindReplacement August 12, 2021 08:56

mstoykov previously approved these changes Aug 12, 2021

View reviewed changes

cmd/run.go Outdated Show resolved Hide resolved

na-- previously approved these changes Aug 12, 2021

View reviewed changes

imiric pushed a commit that referenced this pull request Aug 13, 2021

Show end-of-test summary on engine run error

2381b70

Resolves #2093 (comment)

imiric requested review from mstoykov and na-- August 13, 2021 10:10

mstoykov reviewed Aug 13, 2021

View reviewed changes

Base automatically changed from feat/common.BindReplacement to master August 25, 2021 14:47

mstoykov dismissed stale reviews from na-- and themself via 2381b70 August 25, 2021 14:47

imiric pushed a commit that referenced this pull request Sep 7, 2021

Show end-of-test summary on engine run error

06cc52f

Resolves #2093 (comment)

imiric force-pushed the feat/1001-abort-test branch from 2381b70 to 06cc52f Compare September 7, 2021 14:10

imiric pushed a commit that referenced this pull request Sep 17, 2021

Show end-of-test summary when test is interrupted

710cfed

Resolves #2093 (comment)

imiric force-pushed the feat/1001-abort-test branch from 06cc52f to 710cfed Compare September 17, 2021 09:49

imiric changed the title ~~Core changes for aborting test from script~~ Add JS function to abort test Sep 17, 2021

gernest and others added 8 commits December 17, 2021 10:47

Update k6.abortTest() tests

c24e04e

Update error message and docs

4e21b36

Show end-of-test summary when test is interrupted

b7ab4d2

Resolves #2093 (comment)

Add ExecutionStatusInterrupted, set it in ExecutionScheduler.Run

1bab74e

This is needed to be able to read the test run status via the API, e.g. in the Cloud.

Use new ScriptAborted (108) exit code for test.abort()

52eed6b

Resolves #2093 (comment)

Properly handle test.abort() exit code in all cases

8abac3e

This ensures we exit with code 108 (ScriptAborted) in all places where it might be called from.

Set RunStatus to AbortedUser when test.abort() is called

dfab9bf

See #2093 (review)

imiric dismissed na--’s stale review via dfab9bf December 17, 2021 09:50

imiric force-pushed the feat/1001-abort-test branch from ae7aaee to dfab9bf Compare December 17, 2021 09:50

imiric requested review from mstoykov, na-- and yorugac December 17, 2021 10:03

na-- approved these changes Dec 17, 2021

View reviewed changes

codebien approved these changes Dec 17, 2021

View reviewed changes

na-- added the documentation-needed A PR which will need a separate PR for documentation label Dec 17, 2021

imiric merged commit edd149b into master Dec 17, 2021

imiric deleted the feat/1001-abort-test branch December 17, 2021 13:40

na-- mentioned this pull request Dec 21, 2021

Controlling RPS with Number of iteration for each VU #2305

Closed

oleiade pushed a commit that referenced this pull request Jan 11, 2022

Show end-of-test summary when test is interrupted

1ad2e74

Resolves #2093 (comment)

oleiade pushed a commit that referenced this pull request Jan 11, 2022

Use new ScriptAborted (108) exit code for test.abort()

1f99b6d

Resolves #2093 (comment)

oleiade pushed a commit that referenced this pull request Jan 11, 2022

Set RunStatus to AbortedUser when test.abort() is called

64e6b91

See #2093 (review)

na-- mentioned this pull request Dec 5, 2022

Wait for metrics handling to finish even when there is an error #2798

Merged

harrytwigg pushed a commit to APITeamLimited/globe-test that referenced this pull request Jan 11, 2023

Show end-of-test summary when test is interrupted

6b247ba

Resolves grafana/k6#2093 (comment)

harrytwigg pushed a commit to APITeamLimited/globe-test that referenced this pull request Jan 11, 2023

Use new ScriptAborted (108) exit code for test.abort()

4c688c2

Resolves grafana/k6#2093 (comment)

harrytwigg pushed a commit to APITeamLimited/globe-test that referenced this pull request Jan 11, 2023

Set RunStatus to AbortedUser when test.abort() is called

f273ce4

See grafana/k6#2093 (review)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add JS function to abort test #2093

Add JS function to abort test #2093

imiric commented Jul 13, 2021 •

edited

Loading

codecov-commenter commented Jul 13, 2021 •

edited

Loading

imiric commented Jul 13, 2021

mstoykov left a comment

imiric commented Jul 22, 2021

mstoykov left a comment

na-- left a comment

mstoykov Aug 13, 2021

imiric Aug 13, 2021 •

edited

Loading

mstoykov Sep 13, 2021

imiric Sep 17, 2021

imiric commented Dec 17, 2021

codebien left a comment

imiric commented Dec 17, 2021 •

edited

Loading

na-- commented Dec 17, 2021

jackgoodacre-crypto commented Jan 13, 2022 •

edited

Loading

Add JS function to abort test #2093

Add JS function to abort test #2093

Conversation

imiric commented Jul 13, 2021 • edited Loading

codecov-commenter commented Jul 13, 2021 • edited Loading

Codecov Report

imiric commented Jul 13, 2021

mstoykov left a comment

Choose a reason for hiding this comment

imiric commented Jul 22, 2021

mstoykov left a comment

Choose a reason for hiding this comment

na-- left a comment

Choose a reason for hiding this comment

mstoykov Aug 13, 2021

Choose a reason for hiding this comment

imiric Aug 13, 2021 • edited Loading

Choose a reason for hiding this comment

mstoykov Sep 13, 2021

Choose a reason for hiding this comment

imiric Sep 17, 2021

Choose a reason for hiding this comment

imiric commented Dec 17, 2021

codebien left a comment

Choose a reason for hiding this comment

imiric commented Dec 17, 2021 • edited Loading

na-- commented Dec 17, 2021

jackgoodacre-crypto commented Jan 13, 2022 • edited Loading

imiric commented Jul 13, 2021 •

edited

Loading

codecov-commenter commented Jul 13, 2021 •

edited

Loading

imiric Aug 13, 2021 •

edited

Loading

imiric commented Dec 17, 2021 •

edited

Loading

jackgoodacre-crypto commented Jan 13, 2022 •

edited

Loading