Save test results for each test spec to improve test results reporting #5449
Conversation
|
✔️ Deploy Preview for odo-docusaurus-preview canceled. 🔨 Explore the source changes: a9431f8 🔍 Inspect the deploy log: https://app.netlify.com/sites/odo-docusaurus-preview/deploys/6228e61026acd3000ae3bc89 |
| commonVar.testFailed = CurrentGinkgoTestDescription().Failed | ||
| commonVar.testDuration = CurrentGinkgoTestDescription().Duration.Seconds() |
There was a problem hiding this comment.
Shouldn't these things go in CommonAfterEach()? How can you know the duration the test took to run and it's pass/fail status before it is run?
There was a problem hiding this comment.
You are right, I moved them to CommonAfterEach()
kadel
added
area/testing
tests/helper/helper_generic.go
Outdated
| if _, err = f.WriteString(resultsRow); err != nil { | ||
| fmt.Println("Error: ", err) | ||
| } | ||
|
|
||
| f.Close() |
There was a problem hiding this comment.
Shouldn't this block be only called when L341-344 does not end in error?
There was a problem hiding this comment.
you are right, change made.
rnapoles-rh
force-pushed
the
5430-improve-test-rslts
branch
from
ed63924
to
44935f6
Compare
tests/helper/helper_generic.go
Outdated
| if err != nil { | ||
| fmt.Println("Error: ", err) | ||
| } else { | ||
| defer f.Close() |
There was a problem hiding this comment.
| defer f.Close() | |
| defer f.Close() //#nosec |
This should fix the unit test failure, or optionally you can remove this defer.
This issue has been reported in securego/gosec#714, so perhaps we can update the gosec library and see if that helps. For a quick fix, you can implement the above suggestions.
tests/helper/helper_generic.go
Outdated
| if _, err = f.WriteString(resultsRow); err != nil { | ||
| fmt.Println("Error: ", err) | ||
| } | ||
| f.Close() | ||
| } | ||
|
|
There was a problem hiding this comment.
Optional:
| if _, err = f.WriteString(resultsRow); err != nil { | |
| fmt.Println("Error: ", err) | |
| } | |
| f.Close() | |
| } | |
| _, err = f.WriteString(resultsRow) | |
| if err != nil { | |
| fmt.Println("Error: ", err) | |
| } | |
| if err = f.Close(); err != nil { | |
| fmt.Println("Error: ", err) | |
| } |
Since we are performing write operation, it is advised that it is done in a separate line, because it makes some changes, for a normal read operation, an inline read and error check is fine.
There was a problem hiding this comment.
Suggestion applied
rnapoles-rh
force-pushed
the
5430-improve-test-rslts
branch
from
44935f6
to
95af809
Compare
|
/test v4.9-integration-e2e |
tests/helper/helper_generic.go
Outdated
| if err != nil { | ||
| fmt.Println("Error: ", err) | ||
| } else { | ||
| f.Close() |
There was a problem hiding this comment.
If you close the file, you will not be able to write to it, please remove f.Close() from line 346.
There was a problem hiding this comment.
It's a wide practice to use defer f.Close() so that the file will be closed as the last thing before go runtime is done with this function.
There was a problem hiding this comment.
that f.close was left behind, removed.
| } | ||
| testDate := strings.Split(time.Now().Format(time.RFC3339), "T")[0] | ||
| resultsRow = prNum + "," + testDate + "," + clusterType + "," + commonVar.testFileName + "," + commonVar.testCase + "," + passedOrFailed + "," + strconv.FormatFloat(commonVar.testDuration, 'E', -1, 64) + "\n" | ||
| testResultsFile := filepath.Join("/", "tmp", "testResults.txt") |
There was a problem hiding this comment.
What are we doing with this file? I assume this is not uploaded on any cloud storage bucket.
There was a problem hiding this comment.
Not yet, once this PR gets merged I will update the pipeline script to upload it to a bucket
tests/helper/helper_generic.go
Outdated
| if err = f.Close(); err != nil { | ||
| fmt.Println("Error: ", err) | ||
| } |
There was a problem hiding this comment.
I think if you do what Dharmit suggested in this comment, I think you can remove this portion.
There was a problem hiding this comment.
followed Parthvi's suggestion as the defer was causing unit tests to fail. Now last thing I do in the after each is closing the file.
rnapoles-rh
force-pushed
the
5430-improve-test-rslts
branch
from
1328b35
to
83650b1
Compare
There was a problem hiding this comment.
Code lgtm. @anandrkskd leaving it up to you to approve this.
openshift-ci
bot
added
the
lgtm
openshift-ci
bot
removed
the
lgtm
|
Kudos, SonarCloud Quality Gate passed!
|
|
timeout failure |
|
/lgtm Thank you for the PR, Rodolfo! |
openshift-ci
bot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: valaparthvi The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
redhat-developer#5449) * PoC to improve test results reporting * PoC to improve test results reporting * PoC to improve test results reporting * PoC to improve test results reporting * PoC to improve test results reporting * PoC to improve test results reporting * PoC to improve test results reporting * PoC to improve test results reporting * PoC to improve test results reporting * PoC to improve test results reporting * PoC to improve test results reporting * PoC to improve test results reporting * PoC to improve test results reporting * PoC to improve test results reporting * PoC to improve test results reporting * PoC to improve test results reporting * PoC to improve test results reporting * PoC to improve test results reporting * PoC to improve test results reporting * PoC to improve test results reporting * Save test results for each spec to improve test results reporting * Incorporated feedback from review * Incorporated feedback from review * Removed blank spaces after commas in data row * Incorporated additional feedback * Updated comment to rerun tests * Incorporated feedback to fix unit tests failure and check for err during f.close * Incorporated additional feedback on close command * Edited comment to rerun tests * Edited comment to rerun tests
What type of PR is this:
/kind tests
What does this PR do / why we need it:
Updated CommonAfterEach to save test results for each test spec to improve test results reporting.
Will work in conjunction with the test pipeline to update cumulative test results file to IBM cloud bucket
Which issue(s) this PR fixes:
No related issue
Fixes #5430
PR acceptance criteria:
Unit test
Integration test
Documentation
How to test changes / Special notes to the reviewer: