Make config loading failures visible #726
Conversation
|
Please add regression tests for this |
|
|
||
| def test_invalid_config_file(base_arguments): | ||
| """Ensures invalid config_file produces error code 2.""" | ||
| with pytest.raises(SystemExit) as excinfo: |
There was a problem hiding this comment.
Have you tried
| with pytest.raises(SystemExit) as excinfo: | |
| with pytest.raises(SystemExit, match='^2$'): |
(not sure if it'll work, but it'd be cleaner if yes)
| def test_invalid_config_file(base_arguments): | ||
| """Ensures invalid config_file produces error code 2.""" | ||
| with pytest.raises(SystemExit, match="^2$"): | ||
| cli.get_config(base_arguments + | ||
| ["-c", "test/fixtures/ansible-config-invalid.yml"]) | ||
|
|
||
|
|
||
| def test_missing_config_file(base_arguments): | ||
| """Ensures missing config_file produces error code 2.""" | ||
| with pytest.raises(SystemExit, match="^2$"): | ||
| cli.get_config(base_arguments + | ||
| ["-c", "/dev/null/ansible-config-missing.yml"]) |
There was a problem hiding this comment.
We should parametrize this, you're copy-pasting too many lines. Also, I'd check for the message in stderr using the capsys fixture.
There was a problem hiding this comment.
For 3 tests maybe it would have made sense, but for only two is going to add complexity. Still, I will do it.
Not testing stderr/stdout output was on purpose because the only contract I want to offer is the result code. The content of the error message and where it is sent may change in time, especially as we are likely to do some refactoring and adopt python logging.
lib/ansiblelint/cli.py
Outdated
| "Config file not found '{cfg!s}'.".format(cfg=config_path), | ||
| file=sys.stderr, | ||
| ) | ||
| sys.exit(2) |
There was a problem hiding this comment.
Could you pls move 2 to a global constant like INVALID_CONFIG_RC = 2?
| sys.exit(2) | |
| sys.exit(INVALID_CONFIG_RC) |
I think it'd communicate the intent of the magic literal better. And would improve the consistency too.
| pytest.param("test/fixtures/ansible-config-invalid.yml", id="invalid"), | ||
| pytest.param("/dev/null/ansible-config-missing.yml", id="missing") |
Corrects implementation of config file loading in order to return an error code in case of failure, with a meaningful error message.
| cli.get_config(base_arguments + | ||
| ["-c", config_file]) |
There was a problem hiding this comment.
Nitpick: this doesn't need to be multilined anymore. But won't block the PR if you decide to keep it.
3 participants
Corrects implementation of config file loading in order to return an error code in case of failure, with a meaningful error message.