set correct exit code when running a script with an invalid option

[ariasmn]

What?

We return the correct status code when exiting because of an invalid option.

Why?

Before this change, when running the script in the issue, k6 would exit with a 255 (uint8 equivalent to -1) exit code, but in reality, the exit code should be 104 which is the InvalidConfig code.

Checklist

• I have performed a self-review of my code.
• I have added tests for my changes.
• I have run linter locally (make lint) and all checks pass.
• I have run tests locally (make tests) and all tests pass.
• I have commented on my code, particularly in hard-to-understand areas.

Related PR(s)/Issue(s)

Closes #3759

[ariasmn]

I tried a multiline string here, but couldn't correctly assert it. If there's a correct way of doing this let me know!

[codebien]

Hi @ariasmn,
thanks for your contribution, really appreciated! 🙇

This pull request seems a good start but it isn't yet fully consistent. Check the case below:

$ k6 run --vus 'bad' script.js
ERRO[0000] invalid argument "bad" for "-u, --vus" flag: strconv.ParseInt: parsing "bad": invalid syntax
$ echo $?
255

If we introduce this change then it has to be consistent across all the methods to set options (env vars, flags, source code and stored configuration).

This comment

k6/cmd/config.go

Line 171 in 41db838

// TODO: use errext.WithExitCodeIfNone(err, exitcodes.InvalidConfig) where it makes sense?

here can be a good suggestion. Doing it during the consolidation can be a good way of catching all the errors.

There is another level of configurations which is the scenario config. The code below should provide to you an example:

export const options = {
  scenarios: {
    example_scenario: {
      // name of the executor to use
      executor: 'shared-iterations',
      // common scenario configuration
      startTime: '10s',
      gracefulStop: '5s',
      env: { EXAMPLEVAR: 'testing' },
      tags: { example_tag: 'testing' },

      // executor-specific configuration
      vus: 10,
      iterations: 200,
      maxDuration: '10s',
    },
    another_scenario: {
      /*...*/
    },
  },
};

Furthermore, we need to cover with tests all these levels. They should have at least one test for each case.

[ariasmn]

Hi @codebien

Thanks for the input! That makes sense.
I'm going to take a look at it during this week (if my schedule allows). I'll try to find a correct solution on my own, but I'll let you know if I have any blockers.

[joanlopez]

Pushing it to the next milestone (https://github.com/grafana/k6/milestone/42), as we're close to the next release and I don't think there's a real need to rush it.

[ariasmn]

@codebien I've added more test cases and fixes, but still some questions left, hopefully I'm not bothering you too much:

• I've moved the tests from https://github.com/grafana/k6/blob/master/cmd/tests/cmd_run_test.go. Don't know if now these are in the correct place, since I don't know the difference between both. Let me know if I should move them back.
• I didn't add the test case/fix when we fail to set an option through the flag, like you mentioned in your comment:

$ k6 run --vus 'bad' script.js
ERRO[0000] invalid argument "bad" for "-u, --vus" flag: strconv.ParseInt: parsing "bad": invalid syntax
$ echo $?
255

AFAICT, the error comes from the parsing of the flag done by Cobra. I'm not very familiar with the library itself, so don't know what's the best approach for this. Maybe compare the error returned by Cobra in here? If so, can you provide any example in the codebase?

k6/cmd/root.go

Line 108 in 8efec40

err := c.cmd.Execute()

Also, if you think nothing I've done makes sense/is right, I'd appreciate it if you could give me some pointers on how to do it the right way. The last thing I want is that helping with a PR ends up being more work than doing it yourselves. 😬

[oleiade]

We've discussed it internally, and I believe this is already a good improvement over the current state of things, and 👍🏻 for going through with it.

Thanks a lot for your contribution 🙇🏻

[ariasmn]

We've discussed it internally, and I believe this is already a good improvement over the current state of things, and 👍🏻 for going through with it.

Thanks a lot for your contribution 🙇🏻

Glad that I could help!

[codebien]

Hey @ariasmn,
I confirm we are quite close and we could merge soon this pull request.

The unique request I have is to move all the testdata's files under a dedicated invalidconfig folder (testdata/invalidconfig/<filename>.js/json.

Can you do that, please? 🙇 After that, we're going to merge it.

[ariasmn]

@codebien Done! Let me know if anything else is left to be done before merging.