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

Adding support for supressing UNKNOWN results via --no-unknown #63

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Adding support for supressing UNKNOWN results via --no-unknown #63

wants to merge 1 commit into from

Conversation

mikenowak
Copy link

I have a use case where a bunch of containers that I query via --containers all do not have healthchecks defined, this results in an UNKNOWN return. This PR adds support for supressing these messages.

For the test class, I am not entirely sure if I am doing this right, so any feedback/corrections welcome.

@timdaman timdaman self-requested a review December 12, 2019 14:28
Copy link
Owner

@timdaman timdaman left a comment

Choose a reason for hiding this comment

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

Thank for the contribution! I found a couple issues but I think they are both are fairly simple though if you haven't used pytest before it could take a few minutes longer.

if len(filtered_messages) == 0:
messages_concat = 'OK'
if no_ok:
Copy link
Owner

Choose a reason for hiding this comment

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

If the user passes --no_ok --no_unknown
UNKNOWN will be added to the output even if one was not removed.

@@ -728,7 +730,7 @@ def test_print_results(check_docker, capsys, messages, perf_data, expected):
assert out.strip() == expected


@pytest.mark.parametrize("messages, perf_data, no_ok, no_performance, expected", (
@pytest.mark.parametrize("messages, perf_data, no_ok, no_unknown, no_performance, expected", (
Copy link
Owner

Choose a reason for hiding this comment

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

You added a test parameter but you didn't update the data to match it. Have you used parameterized pytest tests before? It is pretty simple (and also a little ugly).
The string is a ordered list of names for all of the values in each of the tuples that follow. Each tuple in the list describes the setup of a single test case. For example

def test_func(one, two) 
    ...

This executes does something like this

for params in ((1,2),(11,22)):
  one, two = params
  test_func(one=one, two=two)

Can you add the missing entries to the tuples and also add tuples that exercise the code you are changing.

Don't worry if you break something, I can help you out. Also my dev documentation should help you get the test running on your machine.

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