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

test: improve error output of router_check #4392

Merged
merged 4 commits into from
Sep 13, 2018

Conversation

snowp
Copy link
Contributor

@snowp snowp commented Sep 11, 2018

Description: Explains what the different values output means and writes to stderr
instead of stdout. This should make the error output appear in the
testlogs.
Risk Level: Low, change to test error reporting
Testing: bazel test //test/...
Docs Changes: n/a
Release Notes: n/a

Explains what the different values output means and writes to stderr
instead of stdout. This should make the error output appear in the
testlogs.

Signed-off-by: Snow Pettersen <[email protected]>
Signed-off-by: Snow Pettersen <[email protected]>
Signed-off-by: Snow Pettersen <[email protected]>
Copy link
Member

@zuercher zuercher left a comment

Choose a reason for hiding this comment

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

Would you mind fixing a bug with the bad config file section?

I think that if the subshell that runs $PATH_BIN for the bad config file doesn't error out (e.g. the bad config is successfully parsed) then the if statement won't get run because the subshell succeeded. This means the output doesn't get checked.

I think it should use the same pattern as the failure test case, with || echo "${BAD_CONFIG_OUTPUT:-no output}" and the if statement as a separate command.

@@ -26,8 +26,8 @@ BAD_CONFIG_OUTPUT=$(("${PATH_BIN}" "${PATH_CONFIG}/Redirect.golden.json" "${PATH

# Failure test case
echo testing failure test case
FAILURE_OUTPUT=$("${PATH_BIN}" "${PATH_CONFIG}/TestRoutes.yaml" "${PATH_CONFIG}/Weighted.golden.json" "--details") ||
FAILURE_OUTPUT=$("${PATH_BIN}" "${PATH_CONFIG}/TestRoutes.yaml" "${PATH_CONFIG}/Weighted.golden.json" "--details" 2>&1) ||
echo ${FAILURE_OUTPUT}
Copy link
Member

Choose a reason for hiding this comment

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

What do you think of making this echo "${FAILURE_OUTPUT:-no output}"? That will produce a line that says "no output" should that happen.

This line should also be indented two, since it's a continuation of the previous line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds reasonable to me

@snowp
Copy link
Contributor Author

snowp commented Sep 12, 2018

Updated this PR with your suggested fix, ptal.

@zuercher zuercher merged commit 5b0bcb2 into envoyproxy:master Sep 13, 2018
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