-
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 infinite loop in e2e tests #383
Conversation
tests/e2e/e2e_test.go
Outdated
Fail(err.Error()) | ||
} | ||
|
||
outInt, _ := strconv.Atoi(string(out)) |
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.
why are we converting the output to int?
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.
@kadel I need the output in int since I am doing wc -l
comparison
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.
then we need a different name for this function. It is really specific one command, and it won't work for anything that is not ending with wc
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.
@kadel , how about we pass expected value in the function. Something like this:
func pingCmd(cmd string, expOut string) bool {
...
out, err := exec.Command("/bin/sh", "-c", cmd).Output()
if err != nil {
Fail(err.Error())
}
if string(out) == expOut {
return true
}
...
}
pingCmd("curl -s getRoute | grep -i odo | wc -l | tr -d '\n'" , "1")
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.
👍 that looks better.
It might be good to change function name pingCmd
doesn't really indicate what this function does.
Also, don't forget to add a comment with a description what this function is for.
tests/e2e/e2e_test.go
Outdated
} | ||
|
||
outInt, _ := strconv.Atoi(string(out)) | ||
if outInt > 0 { |
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.
what happens when outInt
is 0 or less?
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.
@kadel continues in the loop for 5 minutes
tests/e2e/e2e_test.go
Outdated
for { | ||
select { | ||
case <-pingTimeout: | ||
Fail("timeout out after 5 minutes") |
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.
Shouldn't be this timeout shorter? 5mins is really long time.
How about 1min? Is there a chance that after 1min the command will succeed?
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.
Sure. Have changed it to 1min for now. ALso, have changed the function name to waitForCmdOut
e2927f8
to
e7e4920
Compare
tests/e2e/e2e_test.go
Outdated
@@ -73,6 +73,35 @@ func pingSvc(url string) { | |||
} | |||
} | |||
|
|||
func waitForCmdOut(cmd string, expOut string) bool { | |||
// This function runs the command until it \ |
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.
The comment should be before the function.
Check this out https://blog.golang.org/godoc-documenting-go-code
493dae1
to
c9ed219
Compare
tests/e2e/e2e_test.go
Outdated
@@ -73,6 +73,35 @@ func pingSvc(url string) { | |||
} | |||
} | |||
|
|||
// This function runs the command until it \ |
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.
why \
at the end of each line?
There is a standard in go, that comment for function starts with a function name.
c9ed219
to
c8045c5
Compare
@kadel hope this works. |
Fixes #368