-
Notifications
You must be signed in to change notification settings - Fork 244
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
Fix odo log flake #3733
Fix odo log flake #3733
Conversation
2e1bb20
to
29d8c28
Compare
// It is different from WaitforCmdOut which gives stdout in one go using session.Out.Contents() | ||
// for commands like odo log -f which streams continuous data and does not terminate by their own | ||
// we need to read the stream data from buffer. | ||
func RunCmdWithMatchOutputFromBuffer(timeoutAfter time.Duration, matchString, program string, args ...string) (bool, error) { |
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.
+100
output = helper.CmdShouldRunWithTimeout(1*time.Second, "odo", "log", "-f") | ||
Expect(output).To(ContainSubstring("program=devrun")) | ||
// Test odo log -f | ||
match, err := helper.RunCmdWithMatchOutputFromBuffer(30*time.Second, "program=devrun", "odo", "log", "-f") |
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
// It is different from WaitforCmdOut which gives stdout in one go using session.Out.Contents() | ||
// for commands like odo log -f which streams continuous data and does not terminate by their own | ||
// we need to read the stream data from buffer. | ||
func RunCmdWithMatchOutputFromBuffer(timeoutAfter time.Duration, matchString, program string, args ...string) (bool, error) { |
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.
I was thinking instead of overloading helper file, Can't we use gingko in-build pattern for timeout - https://onsi.github.io/ginkgo/#asynchronous-tests
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.
@prietyc123 I follow the pattern from odo watch
tests, timeout we are using this way only. I agree with your concern, once you update some other tests with gingko in-build
pattern as an example, i would follow the same.
Codecov Report
@@ Coverage Diff @@
## master #3733 +/- ##
=======================================
Coverage 44.93% 44.93%
=======================================
Files 133 133
Lines 12749 12749
=======================================
Hits 5729 5729
Misses 6460 6460
Partials 560 560 Continue to review full report at Codecov.
|
|
||
command := exec.Command(program, args...) | ||
command.Stdout = &buf | ||
|
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.
I think we should also check the StdErr stream. In case of the timeout, we can print the contents from the error buffer. It will help in debugging in case the command fails to stream.
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.
done
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.
I think it's better to use a different buffer for the error stream like in case of the watch test.
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.
done
/retest |
/lgtm |
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.
small nitpick but code looks good to to me.
/approve
select { | ||
case <-timeoutCh: | ||
fmt.Fprintln(GinkgoWriter, errBuf.String()) | ||
return false, errors.New("Timeout waiting for the conditon") |
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.
no capitalization on errors
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cdrage 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 |
/retest
|
/retest Please review the full test history for this PR and help us cut down flakes. |
1 similar comment
/retest Please review the full test history for this PR and help us cut down flakes. |
What type of PR is this?
/kind flake
What does does this PR do / why we need it:
Fixes the flag for
odo log -f
test. This PR updates the test to check the log until expected log is printed and timeout if it doesn't gets logged in specified timeout duration.Which issue(s) this PR fixes:
Fixes #3610
PR acceptance criteria:
Unit test
Integration test
Documentation
How to test changes / Special notes to the reviewer: