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

Fix a bunch of minor issues and repetitions #1453

Merged
merged 2 commits into from
May 18, 2020

Conversation

na--
Copy link
Member

@na-- na-- commented May 18, 2020

I started commenting on a bunch of minor issues in #1428, but then realized that patching them would be simpler than explaining them, even without the back-and-forth comments... 😅

@imiric, if you don't find any issues with this, feel free to merge it in #1428 and then merge your PR.

@na-- na-- requested a review from imiric May 18, 2020 09:01
@na--
Copy link
Member Author

na-- commented May 18, 2020

hmm wait, I think I found another minor issue, investigating...

@na--
Copy link
Member Author

na-- commented May 18, 2020

Previously, running this code with k6 run --env aaa=bbb --include-system-env-vars=false script.js:

export default function () {
  console.log(`Start VU ${__VU} ITER ${__ITER}, ENV ` + JSON.stringify(__ENV));
  __ENV.asdfg = 123123;
}

export let options = {
  execution: {
    scenario1: {
      type: "per-vu-iterations",
      vus: 1,
      iterations: 1,
      maxDuration: "3s",
      env: {
        "111": "111",
      },
      gracefulStop: 0,
    },
    scenario2: {
      type: "per-vu-iterations",
      vus: 1,
      iterations: 1,
      startTime: "3s",
      maxDuration: "10s",
      env: {
        "222": "222",
      },
    },
  },
};

would have outputted something like this:

INFO[0000] Start VU 1 ITER 0, ENV {"aaa":"bbb","111":"111"} 
INFO[0003] Start VU 1 ITER 1, ENV {"aaa":"bbb","111":"111","asdfg":"123123","222":"222"} 

after a345f5c, it will output

INFO[0002] Start VU 1 ITER 0, ENV {"aaa":"bbb","111":"111"} 
INFO[0005] Start VU 1 ITER 1, ENV {"aaa":"bbb","222":"222"} 

Which is much saner, even if it might be considered a slight breaking change. Though probably not, I personally consider relying on __ENV persistence between VU reuses relying on undefined behavior... Also, VU reuses happened only in stages or iterations, so this will actually mostly continue to work, since in the new paradigm, these will be single executors, so __ENV persistence will continue to work like before for these cases.

Anyway, this probably deserves its own minor test, but I don't have time to write it now, so @imiric, feel free to write one (or extend one of your existing ones) if you want.

Copy link
Contributor

@imiric imiric left a comment

Choose a reason for hiding this comment

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

Thanks for the env fix, I didn't consider VU reuse... 🤦 I'll add a test for it before merging #1428.

And cheers for the group tag handling in a single place and where it belongs now, though having to set it in tests is a bit unfortunate, but not a big deal.

@imiric imiric merged commit e1a5c0a into feat/1007-1300-env-tags-exec-options May 18, 2020
@imiric imiric deleted the env-tags-exec-nitpicks branch May 18, 2020 10:09
imiric pushed a commit that referenced this pull request May 18, 2020
This would previously fail without the fixes in #1453.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants