Skip to content

Conversation

rishinair11
Copy link
Contributor

@rishinair11 rishinair11 commented Mar 11, 2023

Fixes #449

What this PR does / why we need it:

When reportFormat in kuttl-test.yaml is specified in uppercase, it is passed unchanged to report.Report func in harness.go.
It needs to be converted to lowercase so that the switch compares against valid report format types (ftype)

This has been taken care of here already.

var ftype = report.Type(strings.ToLower(reportFormat))

Also added unit tests for the report.Report func to verify the behaviour.

Fixes issue kudobuilder#449

When reportFormat in `kuttl-test.yaml` is specified in uppercase,
it is passed as it is to report.Report func in harness.go.
It needs to be passed as lowercase so that
the `switch` compares against valid report format types (`ftype`)

Also added unit tests

List of tests added:

- should_create_an_XML_report_when_format_is_XML
- should_create_an_XML_report_when_format_is_xml
- should_create_an_JSON_report_when_format_is_JSON
- should_create_an_JSON_report_when_format_is_json
- should_not_create_any_report_when_format_is_empty

Signed-off-by: Rishikesh Nair <[email protected]>
}
if err := h.report.Report(h.TestSuite.ArtifactsDir, h.reportName(), report.Type(h.TestSuite.ReportFormat)); err != nil {

reportType := report.Type(strings.ToLower(h.TestSuite.ReportFormat))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Instead of doing the change here, I think what should be modified is the reportType function from pkg/kuttlctl/cmd/test.go to store the report format in lower letters. That will help to not reproduce this problem in other places where pkg/kuttlctl/cmd/test.go is used.

Copy link
Member

Choose a reason for hiding this comment

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

+1 agree with @iblancasa
as this has been awhile from PR to review (my fault)... if I don't see an update today... I will merge (for proper attribution) and modify consistent to iblancasa comments.

Copy link
Member

@kensipe kensipe left a comment

Choose a reason for hiding this comment

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

nice work... I agree with iblancasa and looking for an update... if not, I will merge and mod. thanks again

}
if err := h.report.Report(h.TestSuite.ArtifactsDir, h.reportName(), report.Type(h.TestSuite.ReportFormat)); err != nil {

reportType := report.Type(strings.ToLower(h.TestSuite.ReportFormat))
Copy link
Member

Choose a reason for hiding this comment

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

+1 agree with @iblancasa
as this has been awhile from PR to review (my fault)... if I don't see an update today... I will merge (for proper attribution) and modify consistent to iblancasa comments.

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.

reportFormat in TestSuite

3 participants