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

Include test names for successful tests in verify command output #775

Open
mhanysz opened this issue Feb 15, 2023 · 4 comments
Open

Include test names for successful tests in verify command output #775

mhanysz opened this issue Feb 15, 2023 · 4 comments
Labels
enhancement New feature or request

Comments

@mhanysz
Copy link

mhanysz commented Feb 15, 2023

Observed Behavior

I want to run conftest verify --output junit in my CI pipeline and noticed that each test case in the output gets reported with the identical name. This leads to errors with tools that work with the generated junit XML, such as gitlab's pipeline tests view. Here's the output of the minimal example to reproduce the issue. Note the identical entries for both test cases:

<?xml version="1.0" encoding="UTF-8"?>
<testsuites>
        <testsuite tests="2" failures="0" time="0.000" name="conftest.">
                <properties>
                        <property name="go.version" value="go1.19.5"></property>
                </properties>
                <testcase classname="conftest." name=".\policy\terraform\opentelekomcloud\issue_test.rego" time="0.000"></testcase>
                <testcase classname="conftest." name=".\policy\terraform\opentelekomcloud\issue_test.rego" time="0.000"></testcase>
        </testsuite>
</testsuites>

Expected Behavior

The test case entries in the junit xml generated when calling conftest verify --output junit should have unique names.

Steps to reproduce the Issue

  1. Create file issue.rego with the following content (taken from website example):
package main

deny[msg] {
	proto := input.resource.aws_alb_listener[lb].protocol
	proto == "HTTP"
	msg = sprintf("ALB `%v` is using HTTP rather than HTTPS", [lb])
}
  1. Create file issue_test.rego with the following content (taken from website example and slightly modified):
package main

test_deny_alb_http {
	cfg := parse_config("hcl2", `resource "aws_alb_listener" "lb_with_http" { protocol = "HTTP" }`)
	deny with input as cfg
}

test_deny_alb_https {
	cfg := parse_config("hcl2", `resource "aws_alb_listener" "lb_with_https" { protocol = "HTTPS" }`)
	count(deny) == 0 with input as cfg
}
  1. Run conftest verify --output junit in the same folder (see observed behavior for output)

Configuration under Test

Conftest: v0.39.0
OPA: v0.49.0
OS: Reproduced on Win10 and Alpine Linux

@jpreese jpreese added the bug Something isn't working label Feb 15, 2023
@jalseth
Copy link
Member

jalseth commented Feb 17, 2023

I believe this is the expected behavior when all tests succeed, at least with the current code. Compare to the JSON output and you'll note no rule names from the unit tests are emitted.

[
	{
		"filename": "issue_test.rego",
		"namespace": "",
		"successes": 1
	},
	{
		"filename": "issue_test.rego",
		"namespace": "",
		"successes": 1
	}
]

However, if you have failures, the test names are emitted. Using the example from the website:

test_deny_alb_http {
  cfg := parse_config("hcl2", `
    resource "aws_alb_listener" "lb_with_http" {
      protocol = "HTTP"
    }
  `)
  deny with input as cfg
}

test_deny_alb_https {
  cfg := parse_config("hcl2", `
    resource "aws_alb_listener" "lb_with_https" {
      protocol = "HTTPS"
    }
  `)
  not deny with input as cfg
}

test_deny_alb_protocol_unspecified {
  cfg := parse_config("hcl2", `
    resource "aws_alb_listener" "lb_with_unspecified_protocol" {
      foo = "bar"
    }
  `)
  not deny with input as cfg
}

We get:

<?xml version="1.0" encoding="UTF-8"?>
<testsuites>
	<testsuite tests="3" failures="2" time="0.000" name="conftest.">
		<properties>
			<property name="go.version" value="go1.19.5"></property>
		</properties>
		<testcase classname="conftest." name="issue_test.rego" time="0.000"></testcase>
		<testcase classname="conftest." name="issue_test.rego - data.main.test_deny_alb_https" time="0.000">
			<failure message="Failed" type="">data.main.test_deny_alb_https</failure>
		</testcase>
		<testcase classname="conftest." name="issue_test.rego - data.main.test_deny_alb_protocol_unspecified" time="0.000">
			<failure message="Failed" type="">data.main.test_deny_alb_protocol_unspecified</failure>
		</testcase>
	</testsuite>
</testsuites>

You can see the successful test does not include the rule name, but the failures do. The JSON output mirrors this:

[
	{
		"filename": "issue_test.rego",
		"namespace": "",
		"successes": 1
	},
	{
		"filename": "issue_test.rego",
		"namespace": "",
		"successes": 0,
		"failures": [
			{
				"msg": "data.main.test_deny_alb_https"
			}
		]
	},
	{
		"filename": "issue_test.rego",
		"namespace": "",
		"successes": 0,
		"failures": [
			{
				"msg": "data.main.test_deny_alb_protocol_unspecified"
			}
		]
	}
]

I do not think this is a bug, but we can definitely consider this as an enhancement request.

@mhanysz
Copy link
Author

mhanysz commented Feb 20, 2023

The primary reason I reported this is that the omission of the rule names for successful tests limits the usefulness of the test report. The purpose of a test report is usually:

  • reporting which tests failed, which works well with the current behavior
  • trace how the test suite evolves over multiple test runs by seeing how the set of tests changes
  • facilitate fixing failing tests by providing info about whether the failing test succeeded in the previous test run

Leaving out the rule names for successful tests impedes the latter two, because the reader of the report can't tell the successful tests apart.

The second issue I encountered with this behavior is that Gitlab doesn't handle junitxml files with identical test cases very well. It simply "deduplicates" them and only reports one successful test case, no matter how many are listed in the file. But that's a shortcoming of Gitlab in my opinion.

So even if the current behavior is as expected, I'd like to suggest to change it to also support the latter two purposes of test reports in general.

@jalseth jalseth added enhancement New feature or request and removed bug Something isn't working labels Feb 20, 2023
@jalseth jalseth changed the title conftest verify --output junit reports all testcases with identical name Include test names for successful tests in verify command output Feb 20, 2023
@jalseth
Copy link
Member

jalseth commented Feb 20, 2023

The use case makes sense for the verify command where each rule in a *_test.rego file is testing a specific input for the deny rule, though not the test command for the same reasons in #731 (comment). I've updated the title and switched the type to enhancement to more accurately reflect the request.

This would require updating the CheckResult struct to treat Successes similarly to the other result types.

@fredgate
Copy link

fredgate commented Jan 18, 2024

I have same problem as @mhanysz and agree with its remarks.

I actually find an interest in this for the verify command.
Note that for the stdout output (with report notes), the test names are clearly displayed. But the report parameter is allowed only for stdout ouput (so not junit).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

4 participants