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.

Sign up for GitHub

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

Jump to bottom

k6.check: Fix returned values when iteration is ending #2915

Merged
merged 2 commits into from
Mar 6, 2023
Merged

k6.check: Fix returned values when iteration is ending #2915

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b36bb5c
k6.check: Fix returned values when iteration is ending
mstoykov Feb 14, 2023
52ac5f3
fixup! k6.check: Fix returned values when iteration is ending
mstoykov Feb 14, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 8 additions & 5 deletions js/modules/k6/k6.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -207,6 +207,11 @@ func (mi *K6) Check(arg0, checks goja.Value, extras ...goja.Value) (bool, error)
exc = err
}
}
booleanVal := val.ToBoolean()
if !booleanVal {
// A single failure makes the return value false.
succ = false
}

// Emit! (But only if we have a valid context.)
select {
Expand All @@ -221,20 +226,18 @@ func (mi *K6) Check(arg0, checks goja.Value, extras ...goja.Value) (bool, error)
Metadata: commonTagsAndMeta.Metadata,
Value: 0,
}
if val.ToBoolean() {
if booleanVal {
atomic.AddInt64(&check.Passes, 1)
sample.Value = 1
} else {
atomic.AddInt64(&check.Fails, 1)

// A single failure makes the return value false.
succ = false
}

metrics.PushIfNotDone(ctx, state.Samples, sample)
}

if exc != nil {
return succ, exc
return false, exc
}
}

Expand Down
64 changes: 64 additions & 0 deletions js/modules/k6/k6_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -323,6 +323,30 @@ func TestCheckArray(t *testing.T) {
}
}

func TestCheckContextDone(t *testing.T) {
t.Parallel()
t.Run("true", func(t *testing.T) {
t.Parallel()
tc := testCaseRuntime(t)

tc.testRuntime.CancelContext()
err := tc.run(`globalThis.result = k6.check(null, {"name": ()=>{ return true }})`)
assert.NoError(t, err)
assert.Len(t, metrics.GetBufferedSamples(tc.samples), 0)
assert.True(t, tc.testRuntime.VU.Runtime().Get("result").ToBoolean())
})
t.Run("false", func(t *testing.T) {
t.Parallel()
tc := testCaseRuntime(t)

tc.testRuntime.CancelContext()
err := tc.run(`globalThis.result = k6.check(null, {"name": ()=>{ return false }})`)
assert.NoError(t, err)
assert.Len(t, metrics.GetBufferedSamples(tc.samples), 0)
assert.False(t, tc.testRuntime.VU.Runtime().Get("result").ToBoolean())
})
}

func TestCheckLiteral(t *testing.T) {
t.Parallel()
rt, samples, _ := checkTestRuntime(t)
Expand Down Expand Up @@ -501,3 +525,43 @@ func TestCheckTags(t *testing.T) {
}, sample.Tags.Map())
}
}

type testCase struct {
samples chan metrics.SampleContainer
testRuntime *modulestest.Runtime
}

func testCaseRuntime(t testing.TB) *testCase {
Comment on lines +529 to +534
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#2946 for just how many line changes the refactor of just this, really small module, are.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks! Yeah, I imagined it wouldn't be a short diff, but still, it DRYs things up considerably.

testRuntime := modulestest.NewRuntime(t)
m, ok := New().NewModuleInstance(testRuntime.VU).(*K6)
require.True(t, ok)
require.NoError(t, testRuntime.VU.RuntimeField.Set("k6", m.Exports().Named))

registry := metrics.NewRegistry()
root, err := lib.NewGroup("", nil)
assert.NoError(t, err)
samples := make(chan metrics.SampleContainer, 1000)
state := &lib.State{
Group: root,
Options: lib.Options{
SystemTags: &metrics.DefaultSystemTagSet,
},
Samples: samples,
Tags: lib.NewVUStateTags(registry.RootTagSet().WithTagsFromMap(map[string]string{"group": root.Path})),
BuiltinMetrics: metrics.RegisterBuiltinMetrics(registry),
}
testRuntime.MoveToVUContext(state)

return &testCase{
samples: samples,
testRuntime: testRuntime,
}
}

func (t *testCase) run(script string) error {
defer t.testRuntime.EventLoop.WaitOnRegistered()
return t.testRuntime.EventLoop.Start(func() (err error) {
_, err = t.testRuntime.VU.Runtime().RunString(script)
return err
})
}