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.

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

Refactor the Engine to process metrics independently from the test Run #1390

Merged
merged 6 commits into from
Apr 14, 2020

Conversation

na--
Copy link
Member

@na-- na-- commented Apr 9, 2020

This allows metric processing to still occur, even after the script run has completed. In practice, this allows you to run k6 with the --linger --no-teardown flags, and execute the teardown() function manually through the REST API, after the main test run has completed.

This also fixes #1369 - now a second Ctrl+C will always abort k6, albeit in a very abrupt way. On the other hand, the first Ctrl+C would be more graceful than it currently is, since it will now allow teardown() to be executed.

This allows metric processing to still occur, even after the script run has completed. In practice, this allows you to run k6 with the --linger --no-teardown flags, and execute the teardown() function manually through the REST API, after the main test run has completed.

This also fixes #1369 - now a second Ctrl+C will _always_ abort k6, albeit in a very abrupt way. On the other hand, the first Ctrl+C would be more graceful than it currently is, since it will now allow teardown() to be executed.
@na-- na-- requested review from mstoykov and imiric April 9, 2020 14:50
@codecov-io
Copy link

codecov-io commented Apr 9, 2020

Codecov Report

Merging #1390 into new-schedulers will decrease coverage by 0.03%.
The diff coverage is 52.17%.

Impacted file tree graph

@@                Coverage Diff                 @@
##           new-schedulers    #1390      +/-   ##
==================================================
- Coverage           76.39%   76.36%   -0.04%     
==================================================
  Files                 162      162              
  Lines               12838    12853      +15     
==================================================
+ Hits                 9808     9815       +7     
- Misses               2513     2518       +5     
- Partials              517      520       +3     
Impacted Files Coverage Δ
cmd/run.go 9.28% <0.00%> (-0.16%) ⬇️
lib/execution.go 91.79% <ø> (ø)
core/engine.go 88.03% <85.71%> (-3.42%) ⬇️
core/local/local.go 70.30% <85.71%> (+0.13%) ⬆️
stats/cloud/collector.go 78.70% <0.00%> (+0.61%) ⬆️
lib/helpers.go 100.00% <0.00%> (+4.16%) ⬆️

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 22e8ab0...3f95a23. Read the comment docs.

core/engine.go Outdated Show resolved Hide resolved
core/engine.go Outdated Show resolved Hide resolved
Copy link
Contributor

@mstoykov mstoykov left a comment

Choose a reason for hiding this comment

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

Seems fine to me :).

A bit more code then I hoped for, but looks okay to me. Will probably need to run a lot of tests, but if the automatic ones pass and you say it doesn't die running I am fine with a merge.

All my comments are about things that have little to do with this change and we can do it in another PR

@na-- na-- requested a review from mstoykov April 14, 2020 07:36
Copy link
Contributor

@imiric imiric left a comment

Choose a reason for hiding this comment

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

No major comments from my side either. Not a big fan of the lambdas returned from Engine.Init(), it feels like this should be better integrated somehow. The new contexts complicate things a bit, but it also makes execution clearer, and it was necessary to properly handle linger, so it's OK.

cmd/run.go Outdated Show resolved Hide resolved
cmd/run.go Outdated Show resolved Hide resolved
cmd/run.go Outdated Show resolved Hide resolved
cmd/run.go Show resolved Hide resolved
core/engine.go Show resolved Hide resolved
core/engine.go Show resolved Hide resolved
core/engine.go Show resolved Hide resolved
lib/execution.go Outdated Show resolved Hide resolved
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.

4 participants