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

Implement WaitForResult in tests to eliminate race conditions #136

Merged
merged 39 commits into from
May 16, 2014

Conversation

tiwilliam
Copy link
Contributor

This is an alternative solution to using sleeps in test. WaitForResult will retry the test call every 10ms and timeout after 5s. A sample test call look like this:

testutil.WaitForResult(func() (bool, error) {
    value, err := SomethingToTest()
    return value == "correct", err
}, func(err error) {
    t.Fatalf("Something went wrong: %v", err)
})

@armon
Copy link
Member

armon commented May 7, 2014

I think this is a great way to do it. Great work!

@tiwilliam
Copy link
Contributor Author

Thanks, yeah it seem to work quite fine on Travis, just some files left to patch.

Ping #131.

@tiwilliam
Copy link
Contributor Author

I'm gathering strength to deal with the last failing tests, just 8 more I'm aware of.

@armon
Copy link
Member

armon commented May 16, 2014

@tiwilliam Awesome work. Thanks so much, it'll be great to have the tests be more reliable.

@armon
Copy link
Member

armon commented May 16, 2014

@tiwilliam Actually, I'm going to merge this in now! We can port the tests in master!

armon added a commit that referenced this pull request May 16, 2014
WIP: Proof of concept using `WaitForResult` in tests
@armon armon merged commit baa831d into master May 16, 2014
@tiwilliam
Copy link
Contributor Author

Cool!

@tiwilliam tiwilliam changed the title WIP: Proof of concept using WaitForResult in tests Implement WaitForResult in tests to eliminate race conditions May 16, 2014
@armon armon deleted the f-testutil-package branch June 6, 2014 23:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants