-
Notifications
You must be signed in to change notification settings - Fork 128
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
Add longevity tests #705
Add longevity tests #705
Conversation
Reviewed 23 of 23 files at r1. tests/e2e/common.go, line 44 at r1 (raw file):
What is the reason behind this rename? tests/longevity/checks.go, line 1 at r1 (raw file):
Missing copyrights header. tests/longevity/checks.go, line 104 at r1 (raw file):
That will overwrite a previous value of err without displaying it. It would be better to collect them in slice and return an empty one if everything is ok. tests/longevity/checks.go, line 118 at r1 (raw file):
Commented code leaved in PR. tests/longevity/runner.go, line 2 at r1 (raw file):
2018 tests/longevity/tools.go, line 1 at r1 (raw file):
Missing copyrights header. tests/longevity/vmcontoller.go, line 1 at r1 (raw file):
Missing copyrights header. tests/longevity/vmcontoller.go, line 31 at r1 (raw file):
Why you are adding "\n" there? It's unusual in logs if there is nothing added after it. tests/longevity/vmcontoller.go, line 34 at r1 (raw file):
Same as above. tests/longevity/vmcontoller.go, line 37 at r1 (raw file):
Why there is 5th level instead commonly used above 3? tests/longevity/vmcontoller.go, line 46 at r1 (raw file):
Same as above. tests/longevity/vmcontoller.go, line 49 at r1 (raw file):
Same as above and missing space after ":". Comments from Reviewable |
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.
Reviewable status: 0 of 2 LGTMs obtained
tests/e2e/common.go, line 44 at r1 (raw file):
Previously, jellonek (Piotr Skamruk) wrote…
What is the reason behind this rename?
What rename?
tests/longevity/checks.go, line 1 at r1 (raw file):
Previously, jellonek (Piotr Skamruk) wrote…
Missing copyrights header.
Done.
tests/longevity/checks.go, line 104 at r1 (raw file):
Previously, jellonek (Piotr Skamruk) wrote…
That will overwrite a previous value of err without displaying it. It would be better to collect them in slice and return an empty one if everything is ok.
I only care about last error when loop ended
tests/longevity/checks.go, line 118 at r1 (raw file):
Previously, jellonek (Piotr Skamruk) wrote…
Commented code leaved in PR.
yeah, I'm not sure what to do here. The code is ok but for some reason attach doesn't work. It worked earlier. There will be a bug for it.
tests/longevity/runner.go, line 2 at r1 (raw file):
Previously, jellonek (Piotr Skamruk) wrote…
2018
Done.
tests/longevity/tools.go, line 1 at r1 (raw file):
Previously, jellonek (Piotr Skamruk) wrote…
Missing copyrights header.
Done.
tests/longevity/vmcontoller.go, line 1 at r1 (raw file):
Previously, jellonek (Piotr Skamruk) wrote…
Missing copyrights header.
Done.
tests/longevity/vmcontoller.go, line 31 at r1 (raw file):
Previously, jellonek (Piotr Skamruk) wrote…
Why you are adding "\n" there? It's unusual in logs if there is nothing added after it.
I had printf there earlier and forgot to clean it.
Done
tests/longevity/vmcontoller.go, line 34 at r1 (raw file):
Previously, jellonek (Piotr Skamruk) wrote…
Same as above.
Done.
tests/longevity/vmcontoller.go, line 37 at r1 (raw file):
Previously, jellonek (Piotr Skamruk) wrote…
Why there is 5th level instead commonly used above 3?
hmm, 5 I'm using for debugging. Should I change 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.
Reviewable status: 0 of 2 LGTMs obtained
tests/e2e/common.go, line 44 at r1 (raw file):
Previously, lukaszo (Łukasz Oleś) wrote…
What rename?
scheduleWait to shedulewait. change of case.
tests/longevity/checks.go, line 118 at r1 (raw file):
Previously, lukaszo (Łukasz Oleś) wrote…
yeah, I'm not sure what to do here. The code is ok but for some reason attach doesn't work. It worked earlier. There will be a bug for it.
Ok.
tests/longevity/vmcontoller.go, line 37 at r1 (raw file):
Previously, lukaszo (Łukasz Oleś) wrote…
hmm, 5 I'm using for debugging. Should I change it?
IMO it should be consistent with other places where you have 3.
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.
Reviewable status: 0 of 2 LGTMs obtained
tests/e2e/common.go, line 44 at r1 (raw file):
Previously, jellonek (Piotr Skamruk) wrote…
scheduleWait to shedulewait. change of case.
Done.
tests/longevity/checks.go, line 118 at r1 (raw file):
Previously, jellonek (Piotr Skamruk) wrote…
Ok.
ah, wrong sed. fixing :)
tests/longevity/vmcontoller.go, line 37 at r1 (raw file):
Previously, jellonek (Piotr Skamruk) wrote…
IMO it should be consistent with other places where you have 3.
Done.
tests/longevity/vmcontoller.go, line 46 at r1 (raw file):
Previously, jellonek (Piotr Skamruk) wrote…
Same as above.
Done.
tests/longevity/vmcontoller.go, line 49 at r1 (raw file):
Previously, jellonek (Piotr Skamruk) wrote…
Same as above and missing space after ":".
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.
Reviewed 11 of 11 files at r2.
Reviewable status: 0 of 2 LGTMs obtained
tests/longevity/checks.go, line 118 at r1 (raw file):
Previously, lukaszo (Łukasz Oleś) wrote…
ah, wrong sed. fixing :)
So it looks like last thing for me ;)
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.
Reviewable status: 0 of 2 LGTMs obtained
tests/longevity/checks.go, line 118 at r1 (raw file):
Previously, jellonek (Piotr Skamruk) wrote…
So it looks like last thing for me ;)
should I just remove it for now?
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.
Reviewable status: 0 of 2 LGTMs obtained
tests/longevity/checks.go, line 118 at r1 (raw file):
Previously, lukaszo (Łukasz Oleś) wrote…
should I just remove it for now?
If it's possible - please fix it, or remove comment. Commented code blocks have tend to be more and more smelly over the time. It can be added later, after fixing, as a follow up.
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.
Reviewable status: 0 of 2 LGTMs obtained
tests/longevity/checks.go, line 118 at r1 (raw file):
Previously, jellonek (Piotr Skamruk) wrote…
If it's possible - please fix it, or remove comment. Commented code blocks have tend to be more and more smelly over the time. It can be added later, after fixing, as a follow up.
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.
Reviewed 1 of 1 files at r3.
Reviewable status: 0 of 2 LGTMs obtained, and 1 stale
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.
Reviewed 10 of 23 files at r1, 8 of 11 files at r2.
Reviewable status: 1 of 2 LGTMs obtained
cmd/longevity/longevity.go, line 38 at r2 (raw file):
if err != nil { glog.Infof("Fatal: %v\n", err) os.Exit(1)
Maybe can just use glog.Fatal(err)
? Or, at least, use glog.Errorf
cmd/longevity/longevity.go, line 53 at r2 (raw file):
if err != nil { glog.Infof("Fatal: %v\n", err) os.Exit(1)
Same as above.
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.
Reviewable status: 0 of 2 LGTMs obtained, and 1 stale
cmd/longevity/longevity.go, line 38 at r2 (raw file):
Previously, ivan4th (Ivan Shvedunov) wrote…
Maybe can just use
glog.Fatal(err)
? Or, at least, useglog.Errorf
Done.
cmd/longevity/longevity.go, line 53 at r2 (raw file):
Previously, ivan4th (Ivan Shvedunov) wrote…
Same as above.
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.
Reviewable status: 0 of 2 LGTMs obtained, and 1 stale
cmd/longevity/longevity.go, line 38 at r2 (raw file):
Previously, lukaszo (Łukasz Oleś) wrote…
Done.
glog.Fatal() exits by itself ;(
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.
Reviewable status: 0 of 2 LGTMs obtained, and 1 stale
cmd/longevity/longevity.go, line 53 at r2 (raw file):
Previously, lukaszo (Łukasz Oleś) wrote…
Done.
same ...
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.
Reviewable status: 0 of 2 LGTMs obtained, and 1 stale
cmd/longevity/longevity.go, line 38 at r2 (raw file):
Previously, ivan4th (Ivan Shvedunov) wrote…
glog.Fatal() exits by itself ;(
done
cmd/longevity/longevity.go, line 53 at r2 (raw file):
Previously, ivan4th (Ivan Shvedunov) wrote…
same ...
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.
Reviewed 4 of 4 files at r5.
Reviewable status: 0 of 2 LGTMs obtained, and 1 stale
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.
Reviewable status: 0 of 2 LGTMs obtained, and 2 stale
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.
Reviewable status: 1 of 2 LGTMs obtained, and 1 stale
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.
Reviewed 1 of 1 files at r6.
Reviewable status: 0 of 2 LGTMs obtained, and 2 stale
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.
Reviewable status: 0 of 2 LGTMs obtained, and 2 stale
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.
Reviewed 4 of 4 files at r5, 1 of 1 files at r6.
Reviewable status: 1 of 2 LGTMs obtained, and 1 stale
This change is