Skip to content
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 some extra checking in the tests to prevent flakes. #26123

Merged
merged 1 commit into from May 25, 2016

Conversation

brendandburns
Copy link
Contributor

@brendandburns brendandburns commented May 23, 2016

Attempts to fix #25967

The hypothesis is that somehow waitTest() catches an idle that occurs before all changes have been applied. This will block until the expected number of changes have arrived.

@brendandburns brendandburns added kind/flake Categorizes issue or PR as related to a flaky test. team/cluster labels May 23, 2016
@brendandburns
Copy link
Contributor Author

@jsafrane since he was also looking @ this.

@k8s-github-robot k8s-github-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. release-note-label-needed labels May 23, 2016
@saad-ali saad-ali assigned jsafrane and unassigned saad-ali May 23, 2016
@k8s-bot
Copy link

k8s-bot commented May 24, 2016

GCE e2e build/test passed for commit 88663fc.

@jsafrane
Copy link
Member

LGTM, thanks for the fix!

@jsafrane jsafrane added lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesn't merit a release note. and removed release-note-label-needed labels May 24, 2016
@brendandburns brendandburns added this to the v1.3 milestone May 24, 2016
// Call the tested function
err := test.test(ctrl, reactor, test)
if err != nil {
t.Errorf("Test %q initial test call failed: %v", test.name, err)
}

reactor.waitTest()
for reactor.getChangeCount() < count+expectedChanges[ix] {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@brendandburns Drive-by comment. What prevents this from entering an endless loop?

@alex-mohr
Copy link
Contributor

buildcop says: this seems to make tests better, so marking p2 so it jumps the queue.

@alex-mohr alex-mohr added the priority/backlog Higher priority than priority/awaiting-more-evidence. label May 25, 2016
@ghost
Copy link

ghost commented May 25, 2016

@alex-mohr Can somebody answer my question above before merging this please.
To re-iterate:

  • reactor.waitTest()
    
  • for reactor.getChangeCount() < count+expectedChanges[ix] {
    
    @brendandburns Drive-by comment. What prevents this from entering an endless loop?

Q

@k8s-github-robot
Copy link

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

@k8s-bot
Copy link

k8s-bot commented May 25, 2016

GCE e2e build/test passed for commit 88663fc.

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit 70a7199 into kubernetes:master May 25, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/flake Categorizes issue or PR as related to a flaky test. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/backlog Higher priority than priority/awaiting-more-evidence. release-note-none Denotes a PR that doesn't merit a release note. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

unit test flake: controller/persistentvolume
7 participants