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

remove sleep from tests #2555

Merged
merged 1 commit into from
Mar 24, 2017
Merged

remove sleep from tests #2555

merged 1 commit into from
Mar 24, 2017

Conversation

phemmer
Copy link
Contributor

@phemmer phemmer commented Mar 21, 2017

This removes time.Sleep() from the majority of the tests. This should alleviate the racyness that causes the CircleCI tests to randomly fail. It also makes the affected tests significantly faster.
The few tests where time.Sleep() wasn't removed generally one of 2 cases. Either doing so would have made the code very complex, or I don't have the ability to test the change.

Required for all PRs:

  • CHANGELOG.md updated (we recommend not updating this until the PR has been approved by a maintainer)
  • Sign CLA (if not already signed)

Fixes #2382

@phemmer
Copy link
Contributor Author

phemmer commented Mar 21, 2017

Tests are unfortunately failing on the tail input plugin. The github.com/hpcloud/tail package is racy as hell. Am going to try to submit a patch on it to fix some of the more critical issues. But I can't even get that package's own tests to pass on my system (even on a completely unmodified master), so it might take a little work.

@danielnelson
Copy link
Contributor

We could merge everything except the tail changes now, and fix those tests later.

@@ -60,13 +59,19 @@ func TestTailFromEnd(t *testing.T) {

acc := testutil.Accumulator{}
require.NoError(t, tt.Start(&acc))
time.Sleep(time.Millisecond * 100)
time.Sleep(time.Millisecond * 200) //TODO remove once https://github.com/hpcloud/tail/pull/114 is merged & added to Godeps
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This solves the race detector issues.

@danielnelson danielnelson merged commit 1402c15 into influxdata:master Mar 24, 2017
@phemmer phemmer deleted the no-test-sleep branch March 24, 2017 19:05
ssorathia pushed a commit to ssorathia/telegraf that referenced this pull request Mar 25, 2017
@danielnelson
Copy link
Contributor

@phemmer Getting lots of failures over the last few days from the tail tests, any sense on if this might be fixed by your upstream fix? https://circleci.com/gh/influxdata/telegraf/6404

@phemmer
Copy link
Contributor Author

phemmer commented Mar 28, 2017 via email

@phemmer
Copy link
Contributor Author

phemmer commented Mar 29, 2017

That library is so incredibly messy. If there were an alternative available, I'd recommend switching. But I find no alternatives.
Anyway, found & fixed the issue, upstream PR here: hpcloud/tail#116

Looking at the history of the project, PRs seem to take months to merge. The last PR took 5 months to merge in, and there were no changes that had to be made during review process.
Given that, I'm not sure how you want to proceed. The telegraf tail test failures aren't just the tests failing. They're legitimate issues. We can add hacks to get the tests to pass, but that kinda defeats the point of the tests, as real world users can experience the issues.

@danielnelson
Copy link
Contributor

I think we will have to use a fork at least until your changes are accepted. Would you rather merge both of your changes onto a single branch on your fork or should I make one on our organization?

@phemmer
Copy link
Contributor Author

phemmer commented Mar 29, 2017

I think I would prefer it be part of influxdata, or an official member of the team :-)

@danielnelson
Copy link
Contributor

Can you open the PRs on https://github.com/influxdata/tail. I really appreciate all the help, you are official to me but perhaps not to the legal team ;)

@danielnelson danielnelson mentioned this pull request Mar 29, 2017
3 tasks
calerogers pushed a commit to calerogers/telegraf that referenced this pull request Apr 5, 2017
vlamug pushed a commit to vlamug/telegraf that referenced this pull request May 30, 2017
maxunt pushed a commit that referenced this pull request Jun 26, 2018
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