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

js/data: ModuleV2 migration #2243

Merged
merged 1 commit into from
Nov 25, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
104 changes: 71 additions & 33 deletions js/modules/k6/data/data.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,23 +21,87 @@
package data

import (
"context"
"errors"
"strconv"
"sync"

"github.com/dop251/goja"
"go.k6.io/k6/js/common"
"go.k6.io/k6/lib"
"go.k6.io/k6/js/modules"
)

type data struct {
shared sharedArrays
type (
// RootModule is the global module instance that will create module
// instances for each VU.
RootModule struct {
shared sharedArrays
}

// Data represents an instance of the data module.
Data struct {
vu modules.VU
shared *sharedArrays
}

sharedArrays struct {
data map[string]sharedArray
mu sync.RWMutex
}
)

var (
_ modules.Module = &RootModule{}
_ modules.Instance = &Data{}
)

// New returns a pointer to a new RootModule instance.
func New() *RootModule {
return &RootModule{
shared: sharedArrays{
data: make(map[string]sharedArray),
},
}
}

type sharedArrays struct {
data map[string]sharedArray
mu sync.RWMutex
// NewModuleInstance implements the modules.Module interface to return
// a new instance for each VU.
func (rm *RootModule) NewModuleInstance(vu modules.VU) modules.Instance {
return &Data{
vu: vu,
shared: &rm.shared,
}
}

// Exports returns the exports of the data module.
func (d *Data) Exports() modules.Exports {
return modules.Exports{
Named: map[string]interface{}{
"SharedArray": d.sharedArray,
},
}
}

// sharedArray is a constructor returning a shareable read-only array
// indentified by the name and having their contents be whatever the call returns
func (d *Data) sharedArray(call goja.ConstructorCall) *goja.Object {
rt := d.vu.Runtime()

if d.vu.State() != nil {
common.Throw(rt, errors.New("new SharedArray must be called in the init context"))
}

name := call.Argument(0).String()
if name == "" {
common.Throw(rt, errors.New("empty name provided to SharedArray's constructor"))
}

fn, ok := goja.AssertFunction(call.Argument(1))
if !ok {
common.Throw(rt, errors.New("a function is expected as the second argument of SharedArray's constructor"))
}

array := d.shared.get(rt, name, fn)
return array.wrap(rt).ToObject(rt)
}

func (s *sharedArrays) get(rt *goja.Runtime, name string, call goja.Callable) sharedArray {
Expand All @@ -57,32 +121,6 @@ func (s *sharedArrays) get(rt *goja.Runtime, name string, call goja.Callable) sh
return array
}

// New return a new Module instance
func New() interface{} {
return &data{
shared: sharedArrays{
data: make(map[string]sharedArray),
},
}
}

// XSharedArray is a constructor returning a shareable read-only array
// indentified by the name and having their contents be whatever the call returns
func (d *data) XSharedArray(ctx context.Context, name string, call goja.Callable) (goja.Value, error) {
if lib.GetState(ctx) != nil {
return nil, errors.New("new SharedArray must be called in the init context")
}

if len(name) == 0 {
return nil, errors.New("empty name provided to SharedArray's constructor")
}

rt := common.GetRuntime(ctx)
array := d.shared.get(rt, name, call)

return array.wrap(rt), nil
}

func getShareArrayFromCall(rt *goja.Runtime, call goja.Callable) sharedArray {
gojaValue, err := call(goja.Undefined())
if err != nil {
Expand Down
58 changes: 42 additions & 16 deletions js/modules/k6/data/share_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ package data

import (
"context"
"fmt"
"sync"
"testing"
"time"
Expand All @@ -30,6 +31,7 @@ import (
"github.com/stretchr/testify/require"

"go.k6.io/k6/js/common"
"go.k6.io/k6/js/modulestest"
)

const makeArrayScript = `
Expand All @@ -43,22 +45,33 @@ var array = new data.SharedArray("shared",function() {
});
`

func newConfiguredRuntime(moduleInstance interface{}) (*goja.Runtime, error) {
func newConfiguredRuntime() (*goja.Runtime, error) {
rt := goja.New()
rt.SetFieldNameMapper(common.FieldNameMapper{})
ctx := common.WithRuntime(context.Background(), rt)
err := rt.Set("data", common.Bind(rt, moduleInstance, &ctx))

m, ok := New().NewModuleInstance(
&modulestest.VU{
RuntimeField: rt,
InitEnvField: &common.InitEnvironment{},
CtxField: common.WithRuntime(context.Background(), rt),
StateField: nil,
},
).(*Data)
if !ok {
return rt, fmt.Errorf("not a Data module instance")
}
Comment on lines +60 to +62
Copy link
Contributor

@yorugac yorugac Nov 23, 2021

Choose a reason for hiding this comment

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

Perhaps, not even a request change, but a curiosity question: @codebien, why didn't you use require.True(t, ok) here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

assert|require with concurrency could generate a race so I tend to avoid it for helper functions if it's possible (if the func return error). Otherwise, I would prefer a classic require.True as you suggested.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think what you are talking about @codebien is that t.FailNow and co. are supposed to be called only from the goroutine that is running the test, not by anyone that is spawned by it.

We do ... not do that quite a lot it just so happens that in most cases the failure never happens 😄
I think it's perfectly fine to leave the error instead of using require.True and co., but also in this case I would expect it's perfectly fine to use it as well

Copy link
Contributor Author

@codebien codebien Nov 23, 2021

Choose a reason for hiding this comment

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

that in most cases the failure never happens

Sure, but in case of changes for refactoring, you could get the race.

but also in this case I would expect it's perfectly fine to use it as well

Yep, but I don't know how the helper could be used in the next test we will write. For this reason, I think it makes sense just for helpers shared across multiple tests. Ofc, in a single test I would prefer to use directly the assert function.


err := rt.Set("data", m.Exports().Named)
if err != nil {
return rt, err //nolint:wrapcheck
}
_, err = rt.RunString("var SharedArray = data.SharedArray;")

return rt, err //nolint:wrapcheck
}

func TestSharedArrayConstructorExceptions(t *testing.T) {
t.Parallel()
rt, err := newConfiguredRuntime(New())
rt, err := newConfiguredRuntime()
require.NoError(t, err)
cases := map[string]struct {
code, err string
Expand All @@ -80,6 +93,10 @@ func TestSharedArrayConstructorExceptions(t *testing.T) {
`,
err: "",
},
"not a function": {
code: `var s = new SharedArray("wat3", "astring");`,
err: "a function is expected",
},
}

for name, testCase := range cases {
Expand All @@ -101,13 +118,12 @@ func TestSharedArrayConstructorExceptions(t *testing.T) {
func TestSharedArrayAnotherRuntimeExceptions(t *testing.T) {
t.Parallel()

moduleInstance := New()
rt, err := newConfiguredRuntime(moduleInstance)
rt, err := newConfiguredRuntime()
require.NoError(t, err)
_, err = rt.RunString(makeArrayScript)
require.NoError(t, err)

rt, err = newConfiguredRuntime(moduleInstance)
rt, err = newConfiguredRuntime()
require.NoError(t, err)
_, err = rt.RunString(makeArrayScript)
require.NoError(t, err)
Expand Down Expand Up @@ -153,15 +169,26 @@ func TestSharedArrayAnotherRuntimeExceptions(t *testing.T) {
func TestSharedArrayAnotherRuntimeWorking(t *testing.T) {
t.Parallel()

moduleInstance := New()
rt, err := newConfiguredRuntime(moduleInstance)
require.NoError(t, err)
_, err = rt.RunString(makeArrayScript)
rt := goja.New()
vu := &modulestest.VU{
RuntimeField: rt,
InitEnvField: &common.InitEnvironment{},
CtxField: common.WithRuntime(context.Background(), rt),
StateField: nil,
}
m, ok := New().NewModuleInstance(vu).(*Data)
require.True(t, ok)
require.NoError(t, rt.Set("data", m.Exports().Named))

_, err := rt.RunString(makeArrayScript)
require.NoError(t, err)

// create another Runtime with new ctx but keep the initEnv
rt, err = newConfiguredRuntime(moduleInstance)
require.NoError(t, err)
rt = goja.New()
vu.RuntimeField = rt
vu.CtxField = common.WithRuntime(context.Background(), rt)
require.NoError(t, rt.Set("data", m.Exports().Named))

_, err = rt.RunString(`var array = new data.SharedArray("shared", function() {throw "wat";});`)
require.NoError(t, err)

Expand Down Expand Up @@ -201,9 +228,8 @@ func TestSharedArrayRaceInInitialization(t *testing.T) {
const repeats = 100
for i := 0; i < repeats; i++ {
runtimes := make([]*goja.Runtime, instances)
moduleInstance := New()
for j := 0; j < instances; j++ {
rt, err := newConfiguredRuntime(moduleInstance)
rt, err := newConfiguredRuntime()
require.NoError(t, err)
runtimes[j] = rt
}
Expand Down
18 changes: 9 additions & 9 deletions js/share_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -99,15 +99,15 @@ exports.default = function() {
t.Parallel()
samples := make(chan stats.SampleContainer, 100)
initVU, err := r.NewVU(1, 1, samples)
if assert.NoError(t, err) {
ctx, cancel := context.WithCancel(context.Background())
defer cancel()
vu := initVU.Activate(&lib.VUActivationParams{RunContext: ctx})
err := vu.RunOnce()
assert.NoError(t, err)
entries := hook.Drain()
require.Len(t, entries, 0)
}
require.NoError(t, err)

ctx, cancel := context.WithCancel(context.Background())
defer cancel()
vu := initVU.Activate(&lib.VUActivationParams{RunContext: ctx})
err = vu.RunOnce()
assert.NoError(t, err)
entries := hook.Drain()
assert.Len(t, entries, 0)
})
}
}