-
Notifications
You must be signed in to change notification settings - Fork 87
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
Set reportFormat to lower before generating report #464
base: main
Are you sure you want to change the base?
Conversation
8244b13
to
f3a757f
Compare
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]>
@@ -605,7 +605,9 @@ func (h *Harness) Report() { | |||
if len(h.TestSuite.ReportFormat) == 0 { | |||
return | |||
} | |||
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)) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this 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
@@ -605,7 +605,9 @@ func (h *Harness) Report() { | |||
if len(h.TestSuite.ReportFormat) == 0 { | |||
return | |||
} | |||
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)) |
There was a problem hiding this comment.
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.
Fixes #449
What this PR does / why we need it:
When
reportFormat
inkuttl-test.yaml
is specified in uppercase, it is passed unchanged toreport.Report
func inharness.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.
kuttl/pkg/kuttlctl/cmd/test.go
Line 177 in f6d64c9
Also added unit tests for the
report.Report
func to verify the behaviour.