k6.check: Fix returned values when iteration is ending #2915
Conversation
Before this check was only returning `false` if it also could emit it, and true otherwise. This should rarely be a problem as the same context that is being checked here is the one that interrupts the VM - so no code will be able to run after this. Unfortunately the code checking if it should be emitted is racing with the one that interrupts the VM. So it is possible for the VM to still not be interrupted when a `check` returns a wrong value. It is even possible for more code to be run before the interrupt is actually called. The code still checks the context as this also updates the internal check structure and we don't want to that if the context is done. The above should be changed with #2869 Fixes #2912
Codecov Report
@@ Coverage Diff @@
## master #2915 +/- ##
==========================================
- Coverage 76.92% 76.87% -0.05%
==========================================
Files 225 225
Lines 16873 16875 +2
==========================================
- Hits 12979 12973 -6
- Misses 3062 3069 +7
- Partials 832 833 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
There was a problem hiding this comment.
Nice work tracking this issue! 👏
In hindsight, it seems obvious when reading the code, and it's surprising we haven't run into it until now.
Let's merge this after the v0.43.0 relase, so we can test it properly.
Can you share the script and run options you used to reproduce this? Maybe post it in the issue.
mstoykov
force-pushed
the
fixCheckAtEndOfIteration
branch
from
cb359c6
to
52ac5f3
Compare
|
@imiric I have added tests (I needed more stuff than the current tests needed and didn't want to do a whole test refactor for this ) It is the same as the one in the issue I just decreased the length and added |
| type testCase struct { | ||
| samples chan metrics.SampleContainer | ||
| testRuntime *modulestest.Runtime | ||
| } | ||
|
|
||
| func testCaseRuntime(t testing.TB) *testCase { |
There was a problem hiding this comment.
This seems useful, but don't we already have similar helpers? checkTestRuntime() is very similar.
Can we reuse an existing helper, and even better, move it to somewhere where it's usable by other packages? This is a generic helper that I'm sure we're duplicating elsewhere, as this setup boilerplate is very common.
There was a problem hiding this comment.
I need more stuff than what is available in checkTestRuntime - specifically cancelling the context.
I really didn't want to touch checkTestRuntime as this change "ripples" through all the tests using it. And I also didn't want this bugfix PR to be 90% me refactoring test code.
This is a generic helper that I'm sure we're duplicating elsewhere, as this setup boilerplate is very commo
We are but in every case we need something additional for each module separately.
As part of the ESM saga I would like to be able to test the module loading and resolution in tc39 and also to be usable from other k6 modules (and extensions as consequence). As part of that I will likely try to refactor all of the helpers through k6 modules at least a little bit, hopefully most will be pretty straightforward 🤞
But that will be a huge PR or a bunch of pretty big ones as in most cases this helpers are used a lot and then their result(s) are used even more.
There was a problem hiding this comment.
#2946 for just how many line changes the refactor of just this, really small module, are.
There was a problem hiding this comment.
Thanks! Yeah, I imagined it wouldn't be a short diff, but still, it DRYs things up considerably.
Before this check was only returning
falseif it also could emit it, and true otherwise.This should rarely be a problem as the same context that is being checked here is the one that interrupts the VM - so no code will be able to run after this.
Unfortunately the code checking if it should be emitted is racing with the one that interrupts the VM. So it is possible for the VM to still not be interrupted when a
checkreturns a wrong value. It is even possible for more code to be run before the interrupt is actually called.The code still checks the context as this also updates the internal check structure and we don't want to that if the context is done. The above should be changed with #2869
Fixes #2912