Check data returned from setup() to ensure it contains only data, no functions.

[vtanakas]

Apparently, if the object returned from setup() is an object that contains a function, or contains an object that contains a function (etc...) then that function is quietly stripped from the object, making it unavailable in the default() function call.

Instead, between the call to setup() and default(), the object returned from setup() should be examined and if it contains any non basic data type fields, an exception should be raised and the test aborted.

Alternatively, the data object could be provided to default() with its functions intact, but I imagine there may be other problems with that.

From eventdb.js

export class EventDB {
    constructor(dbHost) {
        this.dbHost = dbHost;
    }

    writeLogin(serverId, userId) {
        writeEvent(this.dbHost, {
            eventType:"login",
            serverId:serverId,
            userId:userId,
            time:Date.now()
        });
    }
}

From test.js

export function setup() {
    let eventDb = new eventdb.EventDB(EVENT_DB_HOST);
    let my_retval = {"db":eventDb };
    return my_retval;
}

export function default(globalData) {
    // Throws errror message, because of undefined member writeLogin.
    globalData["db"].writeLogin("server1", "user1");
}

[robingustafsson]

Ah, good catch. Yes, we should definitely raise an exception if data returned by setup() contains anything but plain old data.

We need the data returned by setup() to be independent from the machine and k6 instance/process where setup() is executed. When running in a clustered/distributed execution mode there might be several hundred machines involved in running the test, and only one server will run the setup()/teardown() but all machines will need the data returned by setup() to be passed to the default function.

[luizbafilho]

I don't think to raise an exception would be the better approach, since the data in that object is still accessible. So, if you try the following you would get a response:

export default function(globalData) {
  console.log(globalData["db"].dbHost)
};

I guess a better way would document that functions won't be available as part of the data passed to the default function via setup return.

[luizbafilho]

@robingustafsson and @na-- what do you think?

[na--]

It seems that currently the data returned by setup() is intentionally marshaled to JSON and straight back to a Go interface{}. It's not very clear why from the commit (:smile:) message, but it's fairly obvious that this is used as a way to remove any non-data parts of the result. As @robingustafsson mentioned, for cloud/clustered execution the returned data needs to be independent from the actual machine. I'd say that even for local execution, if setup() returns a purely JS function, it probably won't work correctly if it's directly passed from the original goja runtime that executed setup() to the other VU runtimes.

@luizbafilho, I agree that we should update the docs to mention this, but I think that it's also probably worth it to either emit a warning or exit with an error if users return non-data values from setup()... We can't "throw an exception" since this would be better handled outside of the JS runtime. My only concern is that it would be somewhat complicated and ugly to implement - we probably have to recursively use reflection on v.Export() or use the properties of the v goja Object to look for functions, which doesn't sound like a fun exercise... I'm also somewhat concerned about correctly handling corner cases like the difference between returning pure JS functions and objects (which is bad) and returning k6 Go objects that have methods, which may be bad, but maybe it's not - the http.response object for example...

[na--]

Closing this, since #799 was merged today. I also updated the docs to mention that only data is passed from setup and created this issue for the remaining incorrect JSON encoding of internal k6 objects.