set correct exit code when running a script with an invalid option #3783
Conversation
ariasmn
requested review from
oleiade and
codebien
and removed request for
a team
cmd/tests/cmd_run_test.go
Outdated
| export default function () {} | ||
| `, | ||
| expLog: []string{ | ||
| `got event Exit with data '&{Error:could not initialize '-': could not load JS test 'file:///-': strconv.ParseInt: parsing \"this is an invalid type\": invalid syntax}'`, |
There was a problem hiding this comment.
I tried a multiline string here, but couldn't correctly assert it. If there's a correct way of doing this let me know!
ariasmn
force-pushed
the
fix/invalid-config-exit-code
branch
from
3aee2e3
to
5265061
Compare
There was a problem hiding this comment.
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
Line 171 in 41db838
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.
|
Hi @codebien Thanks for the input! That makes sense. |
|
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. |
|
@codebien I've added more test cases and fixes, but still some questions left, hopefully I'm not bothering you too much:
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? Line 108 in 8efec40 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. 😬 |
Glad that I could help! |
There was a problem hiding this comment.
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.
|
@codebien Done! Let me know if anything else is left to be done before merging. |
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,
k6would exit with a255(uint8 equivalent to-1) exit code, but in reality, the exit code should be104which is theInvalidConfigcode.Checklist
make lint) and all checks pass.make tests) and all tests pass.Related PR(s)/Issue(s)
Closes #3759