-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Integration test improvements and a few bugfixes #2789
Conversation
Before this, an integration test might have specified an expected non-zero exit code, but the test would have succeeded even if k6 finished normally, with a 0 exit code, by just returning from the main function and not calling os.Exit() explicitly.
d77c1dc
to
b5c68fd
Compare
a3b61e6
to
a6823b2
Compare
I added one more commit to fix a data race and mask a few others, where goroutines spawned by As I wrote in the FIXME and TODO comments, the proper fix will have to wait until #2790 and #1889 have been implemented, hopefully in the next cycle. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left a couple of non-blocking nitpicks. Really great work, glad to see those improvements 👏🏻
a6823b2
to
b37b0dd
Compare
b37b0dd
to
94bafd6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work! 👏 Just left some minor comments.
The Engine didn't wait for the run_status to be properly propagated to the outputs before it finished executing, leading to a potential data race.
These tests were more useful than expected, found another data race in the current This will also be fixed in the new architecture, it's just one more reason why the current Engine architecture sucks. Still, the fix was relatively easy (129fdb6), so I chose to do it rather than disable the test that triggered it. |
Split among the first 3 commits are various improvements and additions to the existing integration tests:
run_status
andresult_status
values ink6 run --out cloud
testsThe 4th commit actually fixes #2788, a small bug I noticed while trying to remove the whole
core.Engine
(#1889). However, because theEngine
removal will likely be merged at the start of the v0.43.0 cycle and because the bug was easy to fix even in the current implementation, I decided to fix it now.This way we'll get the fix released in k6 v0.42.0 and I'll have the test already present in
master
, in preparation for the bigger PR.The 5th commit fixes a minor data-race with the progressbars and masks a few others with dangling goroutines after errors in the init phase, see #2789 (comment) for details. And the 7th commit fixes another minor data race in the old
Engine
, see #2789 (comment).