Skip to content
This repository has been archived by the owner on Sep 17, 2021. It is now read-only.

Add test.abort() function #3

Closed
wants to merge 2 commits into from
Closed

Add test.abort() function #3

wants to merge 2 commits into from

Conversation

imiric
Copy link
Contributor

@imiric imiric commented Jul 13, 2021

This is the JS API that exposes the core changes in grafana/k6#2093, adapted from grafana/k6#1920. The tests are slightly changed and some didn't make it (the ones for cmd), but they're in the history of grafana/k6#2093 so we can bring them back once this extension is moved to core.

It's based on #2 so let's merge that first.

@imiric imiric requested review from mstoykov, codebien and na-- July 13, 2021 13:46
rt := common.GetRuntime(ctx)
if rt == nil {
return nil, errors.New("goja runtime is nil in context")
}

ti := map[string]func() interface{}{
// stop the test run
"abort": func() interface{} {
return func(msg goja.Value) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's a slight issue with this approach.

This script:

import exec from 'k6/x/execution';

exec.test.abort;

export default function () {
  console.log(exec.test.duration);
}

Outputs:

ERRO[0000] getting test information in the init context is not supported
        at file:///tmp/test.js:6:15(7)  executor=per-vu-iterations scenario=default source=stacktrace

This happens on the test.duration access, so it seems that accessing test.abort in init caches the nil *ExecutionState. Actually calling test.abort() or commenting out that line works as expected.

Choose a reason for hiding this comment

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

I do think that you should just move to getting the ctx and rt in each of the callbacks so it's always just get the current one. Hopefully with the new Modules this will be even cheaper and already the whole goja reflection parts are probably way more expensive

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So with the rebase on #2 this is no longer an issue 🎉

I'm not sure if it's because of the ModuleV2 change or the removal of Proxy/DynamicObject, but I didn't want to do any additional changes, as it doesn't seem necessary to get ctx and rt in each function. We do need a fresh ExecutionState though, but that was required before this update.

@imiric imiric requested a review from mstoykov August 6, 2021 15:20
Base automatically changed from feat/getters-begone to main August 26, 2021 12:51
@imiric
Copy link
Contributor Author

imiric commented Sep 17, 2021

This is superseded by grafana/k6#2093 and won't be merged.

@imiric imiric closed this Sep 17, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants