fix: proper error exec.scenario.* in not init context

[olegbespalov]

What?

Always check if scenarioState is available for the execution.scenario, otherwise erroring.

Why?

It makes no sense to use this execution.scenario not inside the VU context.

Closes: #2545

[na--]

I am not sure throwing an exception is the right thing to do here 🤔 We are pretending that the things in k6/execution are properties, and this doesn't seem very idiomatic. Usually, when some object property in JS is unavailable, it is just not there, so attempting to access it will result in an undefined result and not an exception, right?

[olegbespalov]

Hey @na-- , thanks for the input!

We are pretending that the things in k6/execution are properties, and this doesn't seem very idiomatic.

Not sure if I had a chance to learn it from somewhere 🤷

Moreover it a bit contradicts the existing logic, like if you try to access the properties: "name", "executor", "startTime", "progress" you get the error, not the undefined or default value.

The change is also just extending the same behavior to the "iterationInInstance", "iterationInTest" properties.

So, I'm a bit confused 🤔

[mstoykov]

I don't think that throwing exception on property access is unidiomatic. Move over in this particular case the idea is mostly that exec.scenario is "undefined" and you are trying to access a property of it - which will lead to an exception either way.

From a UX perspective I would also argue that it will be a lot easier to debug when exec.scenaro.name throws an exception instead of returning you a "" or null or something.

[mstoykov]

LGTM

[na--]

We are pretending that the things in k6/execution are properties, and this doesn't seem very idiomatic.

Not sure if I had a chance to learn it from somewhere shrug

I just meant that the API is exec.scenario.name, not something like exec.scenario.getName(). So the elements of k6/execution are properties, not methods (with the exception of test.abort()) and they should behave like normal JS properties, if possible.

Moreover it a bit contradicts the existing logic, like if you try to access the properties: "name", "executor", "startTime", "progress" you get the error, not the undefined or default value.

The change is also just extending the same behavior to the "iterationInInstance", "iterationInTest" properties.

So, I'm a bit confused thinking

Yeah, very good points 🤔 But instead of making this work like them, I kind of thing we should also fix them. I don't see a reason why this should actually fail:

import exec from 'k6/execution';

console.log(JSON.stringify(exec, null, 4));

export default function () {
    console.log(JSON.stringify(exec, null, 4));
}

it should just print the execution properties that were available in each context, something like this:

// Init context
{
    "instance": {
        "iterationsCompleted": 0,
        "iterationsInterrupted": 0,
        "vusActive": 0,
        "vusInitialized": 0,
        "currentTestRunDuration": 0
    },
    "test": {
        "options": { /* ... */ }
        // abort() is not printed by JSON.stringify(), but it's there
    },
    "vu": {
        "idInInstance": 0,
        "idInTest": 0
    }
} 

// VU context
{
    "instance": {
        "iterationsCompleted": 0,
        "iterationsInterrupted": 0,
        "vusActive": 1,
        "vusInitialized": 1,
        "currentTestRunDuration": 0.385693
    },
    "scenario": {
        "name": "default",
        "executor": "per-vu-iterations",
        "startTime": 1655191933532,
        "progress": 0,
        "iterationInInstance": 0,
        "iterationInTest": 0
    },
    "test": {
        "options": { /* ... */ }
        // abort is not printed by JSON.stringify(), but it's there
    },
    "vu": {
        "idInInstance": 1,
        "idInTest": 1,
        "iterationInInstance": 0,
        "iterationInScenario": 0,
        "tags": {
            "group": "",
            "scenario": "default"
        }
    }
} 

...or something like that 🤷‍♂️ Take a look at some of the ideas in #2259 and #2260, things like the CWD or a better way to access __ENV should be available in the init context.

[olegbespalov]

@na--

But instead of making this work like them, I kind of thing we should also fix them.

import exec from 'k6/execution';

console.log(JSON.stringify(exec, null, 4));

export default function () {
    console.log(JSON.stringify(exec, null, 4));
}

It's an excellent example, but the question do you think this is a real-world example?

I mean, as @mstoykov said, the UX aspect is essential. So basically, if we apply your suggestion instead of the precise error about using accessing some certain data, we start getting the property undefined in the example below.

import { scenario } from 'k6/execution';

export function setup() {
  console.log(scenario.iterationInTest)
}

export default () => { }

Is it worth it? 🤔

[na--]

Very good points... 🤔 I am not sure 🤷‍♂️ We probably need some actual JS developers to chime in here, to tell us which way follows the principle of least surprise...

[mstoykov]

If we want this in v0.39.0 - I would argue keeping the current behaviour of throwing an exception is the only way forward. Otherwise, the changes required are way too big and will likely need a lot more testing and discussion.

I propose that we merge this as is, and open an issue to discuss potential changes. This change in practice only prevents a rare panic in a case where the API should not be used either way 🤷 .

[olegbespalov]

We probably need some actual JS developers to chime in here

I'd argue 😄 With all my respect to JS developers, the target audience is load-test users, no?

[na--]

If we want this in v0.39.0 - I would argue keeping the current behaviour of throwing an exception is the only way forward. Otherwise, the changes required are way too big and will likely need a lot more testing and discussion.

I propose that we merge this as is, and open an issue to discuss potential changes. This change in practice only prevents a rare panic in a case where the API should not be used either way shrug .

Agreed 👍 Relaxing this constraint ("throw an exception when accessing an invalid property") in the future won't even be a breaking change, if we want to eventually do that... 🤔 So no need to figure it out now.

We probably need some actual JS developers to chime in here

I'd argue smile With all my respect to JS developers, the target audience is load-test users, no?

Disagree about this though. Consistent and predictable APIs are useful for any user, long-time JS developers or no 😄

[na--]

To get back to this specific fix, I notice that it actually changes the "laziness" properties of exec.scenario. Before getScenarioState() was called on every property access, now it's called only once, and I am not sure that doesn't have any implications 🤔

[allansson]

We probably need some actual JS developers to chime in here

I'd argue 😄 With all my respect to JS developers, the target audience is load-test users, no?

Isn't the whole point using JS to get more people into load testing by using a language that a lot of people already know? Otherwise we could've rolled our own language, used XML or whatever.

Either way, we are using JS and with every language comes a set of expectations. If I'm writing an api in Go, I don't use snake_case and I know that my methods should return errors as the second element in a tuple and that it should either be the error or nil. It would be very frustrating if the arguments were swapped, because it subverts my expectation of what should happen.

People learning JS just to use k6 are probably gonna go to some generic JS tutorial to learn the fundamentals, not some k6 tutorial, so their expectations are most likely being set by the more general JS community. And even if they are learning it from k6 tutorials there is a value in staying idiomatic because their knowledge will be transferable to general JS development.

We don't want k6 to be the PHP of JS-runtimes, right? 😅

So I'm with @na-- on this one. I can't remember a single case where an error was thrown because I was trying to read a property. Throwing while setting a property would be fine. Assignment is a side-effect and you can expect some validation to occur. But reading in general is safe (as long as you are checking for undefined, of course).

IMO, the idiomatic way of preventing access at the global scope would've been to, instead of exporting an object, export a function from "k6/execution" either returning the execution-object or throwing an error if the function was called in an invalid context (e.g. the global scope).

[w1kman]

My 2 cents...

In general, I'd expect an object (not null) to not throw when trying to access a property (existing or non-existing).

But I have no problem with an error in case it was thrown to stop me from "CLEARLY doing something wrong" (rather than spending time trying to figure out why undefined was returned). That would also require the error message to be super helpful.

With that said, I'd still rather have a getter function to throw than a getter property.

import { getScenario } from 'k6/execution';

export function setup() {
  const scenario = getScenario() // I'd expect the code to go BOOM here
  console.log(getScenario.iterationInTest) // not here
}

export default () => { }

Or

import { scenario } from 'k6/execution';

export function setup() {
  console.log(scenario.getIterationInTest())
}

export default () => { }

If I was to use a getter property (get propertyName() { ... }) that throws, I'd create a custom .toJSON, to make sure it didn't throw on serialization.

[olegbespalov]

Hey @allansson

Thanks for the input. All that you said is valid, with one critical remark, and the @w1kman well defines this remark.

But I have no problem with an error in case it was thrown to stop me from "CLEARLY doing something wrong" (rather than spending time trying to figure out why undefined was returned). That would also require the error message to be super helpful.

It saves the user time, which means it's a better UX of the k6 tool.

If you are saying that it's wrong to throw an error from the property, well, that probably means that it shouldn't be property (which it is right now) 🤷 But I believe our users will be happier if they know sooner if the things that they are trying to do is not the right path.

[codebien]

Could we apply the usual --throw logic? Raise an error if --throw is set and print a warning (or just return undefined) in the opposite?

[olegbespalov]

To get back to this specific fix, I notice that it actually changes the "laziness" properties of exec.scenario. Before getScenarioState() was called on every property access, now it's called only once, and I am not sure that doesn't have any implications thinking

So I addressed your concern in reverted the change. If we still want just fix the panic in the current release, this PR is only about fixing the panic, nothing more.

Could we apply the usual --throw logic? Raise an error if --throw is set and print a warning (or just return undefined) in the opposite?

We can, but it makes sense if it will be consistent across all API (or at least k6/execution), so that's why the next paragraph.

Keeping in mind all other input I have a strong feeling that any other change deserves a separate issue where we can discuss the API first and make a decision that we can use as the standard across all our APIs. The decision that respects the JS API and also has a good UX.

[na--]

LGTM except the minor nitpick with the error message

[na--]

For anyone who was part of the broader discussion above, or who stumbles on it in the future, @olegbespalov opened this issue where we can continue it: #2574