[WIP] Add a common utility function which makes debugging test failures #161
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
easier and update one of the test files as an example. This makes debugging failures easier since we now include stdout, stderr and exit code for each bash command we run for which we save output to a variable. Previously we didn't do that so if an assertion failed you had no clue what was going on.
Kami
changed the title
arm4b
suggested changes
May 16, 2019
There was a problem hiding this comment.
Hey, we're doing something wrong initially.
Normal BATS commands when invoked via built-in run outputs and formats everything nicely.
First can we remove this hack and use native BATS run?
-RESULT=$(run_command_and_log_output st2 run examples.python_runner_print_python_version -j)
+run st2 run examples.python_runner_print_python_version -jWe can access result of that command via built-in populated $output var by BATS and so RESULT=$() is not needed. What I'm saying it's all already implemented in BATS.
Next, because we're invoking run again later
run eval "echo '$RESULT' | jq -r '.result.stdout'"
$output most probably gets re-populated with the new data and so that's why we're seeing different output when assert is called.
Instead of that, can we create a helper BATS function around jq to avoid eval echo | jq hacks and utilize BATS in a native/documented way?
Additionally, avoid where possible running commands or mutating output after initial built-in Bats run.
As we're integrating new framework, it's important to start doing it right and thoughtful, otherwise hacks and bad practices will hit us back in future.
arm4b
reviewed
May 16, 2019
| assert_output --partial "Python 3." | ||
|
|
||
| RESULT=$(st2 run examples.python_runner_print_python_version -j) | ||
| RESULT=$(run_command_and_log_output st2 run examples.python_runner_print_python_version -j) |
There was a problem hiding this comment.
Suggested change
| RESULT=$(run_command_and_log_output st2 run examples.python_runner_print_python_version -j) | |
| run st2 run examples.python_runner_print_python_version -j |
arm4b
reviewed
May 16, 2019
| run eval "echo '$SETUP_VENV_RESULTS' | jq -r '.result.result'" | ||
| assert_success | ||
|
|
||
| assert_output "Successfully set up virtualenv for the following packs: examples" |
There was a problem hiding this comment.
For example, in this scenario avoiding another run and mutating output will help to preserve original command results. And so we'll do assert against the original output, step with jq filtering here is overcomplication:
- run eval "echo '$SETUP_VENV_RESULTS' | jq -r '.result.result'"
- assert_success
- assert_output "Successfully set up virtualenv for the following packs: examples"
+ assert_output --partial "Successfully set up virtualenv for the following packs: examples"
There was a problem hiding this comment.
That change is fine, but in addition to checking for a specific string, we should also check that the output is valid JSON.
One of the commands in one of the BATS tests returned a JSON array concatenated to a JSON dict:
[
"example_element_1",
"example_element_2"
]
{
"example_key_1": "example_value_1",
"example_key_2": "example_value_2"
}Both of those objects individually are valid JSON, but simply concatenating them together produces invalid JSON.
So we do still need some way to validate that the produced output is valid JSON. This might be good to do in a custom assert_ function, and could even simply wrap jq.
But however we check for JSON validity, this simplification is a good one.
arm4b
reviewed
May 16, 2019
| SETUP_VENV_RESULTS=$(st2 run packs.setup_virtualenv packs=examples python3=true -j) | ||
| SETUP_VENV_RESULTS=$(run_command_and_log_output st2 run packs.setup_virtualenv packs=examples python3=true -j) | ||
| run eval "echo '$SETUP_VENV_RESULTS' | jq -r '.result.result'" | ||
| assert_success |
There was a problem hiding this comment.
In this specific case, if we really need jq, this construct with run | jq + assert should be replaced in favor of creating custom assert_output_jq function that will help us to avoid overriding original test results/outputs.
There was a problem hiding this comment.
I just read this comment after coming to the same conclusion. 👍
|
If we do assert on original CMD, bats will show everything nicely. Here is an example with Similar happens if original CMD fails I mean this all functionality is already present in BATS. |
|
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This pull request establishes a better pattern for handling tests where we need to store shell command output in a variable before processing it / asserting on it.
Without this change it's very hard / impossible to debug test failures which are result of a shell command which output is stored to a variable. Only way to debug those would be to re-run the tests and add additional log / print statements or similar (and in a lot of scenarios that's very slow and expensive and means waiting 30 minutes or more).
This pull request adds a new helper function which prints command output (stdout, stderr, return code / exit code) to stderr. This is a similar pattern to the one we had in robot and makes debugging much easier and faster since you immediately have access to command output on failure.
Before:
Tests failed. Why? Who knows. No way to know without having access to that command output and at the very least this means re-running tests which is a huge waste of time.
With this change:
Resolves #160.