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

try tweaking some times to reduce flakiness #558

Merged
merged 5 commits into from
Feb 23, 2022

Conversation

BinaryFissionGames
Copy link
Contributor

Using this PR to try testing some changes to tests to reduce flakiness, since they don't seem to flake locally.

@codecov
Copy link

codecov bot commented Feb 16, 2022

Codecov Report

Merging #558 (c97bd1a) into master (3b23e5c) will decrease coverage by 1.95%.
The diff coverage is 73.17%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #558      +/-   ##
==========================================
- Coverage   76.69%   74.73%   -1.95%     
==========================================
  Files         132      132              
  Lines        8308     9993    +1685     
==========================================
+ Hits         6371     7468    +1097     
- Misses       1473     2056     +583     
- Partials      464      469       +5     
Impacted Files Coverage Δ
operator/buffer/buffer.go 75.86% <ø> (-1.92%) ⬇️
operator/buffer/memory.go 71.60% <ø> (-3.74%) ⬇️
...builtin/input/aws/cloudwatch/cloudwatch_persist.go 75.00% <ø> (-6.82%) ⬇️
operator/builtin/input/azure/event_hub.go 0.00% <0.00%> (ø)
operator/builtin/input/azure/event_hub_config.go 90.91% <ø> (-1.09%) ⬇️
operator/builtin/input/file/finder.go 100.00% <ø> (ø)
operator/builtin/input/file/reader.go 53.05% <0.00%> (-5.73%) ⬇️
operator/builtin/input/http/config.go 97.56% <ø> (-0.29%) ⬇️
operator/builtin/input/journald/journald.go 58.76% <ø> (-1.83%) ⬇️
operator/builtin/input/udp/udp.go 73.49% <ø> (-1.51%) ⬇️
... and 156 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cd2849f...c97bd1a. Read the comment docs.

@BinaryFissionGames
Copy link
Contributor Author

Ran 5 successful tests in a row; Empirically, using the tests cases exactly as the are from https://github.com/open-telemetry/opentelemetry-log-collection seems much more stable.

It's not 100% transparent to me why it's more stable, however.

@BinaryFissionGames BinaryFissionGames marked this pull request as ready for review February 18, 2022 14:18
@BinaryFissionGames BinaryFissionGames requested a review from a team as a code owner February 18, 2022 14:18
dehaansa
dehaansa previously approved these changes Feb 18, 2022
cpheps
cpheps previously approved these changes Feb 18, 2022
Copy link
Contributor

@cpheps cpheps left a comment

Choose a reason for hiding this comment

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

The fact that just tweaking time values makes it work is a bit of a code smell but I'm ok with this being a temporary mitigation.

cpheps
cpheps previously approved these changes Feb 18, 2022
@armstrmi armstrmi self-requested a review February 18, 2022 15:40
@BinaryFissionGames
Copy link
Contributor Author

https://github.com/observIQ/stanza/runs/5249699444 Another failure, on windows of course. I'll come back to this in a bit.

armstrmi
armstrmi previously approved these changes Feb 18, 2022
@BinaryFissionGames
Copy link
Contributor Author

@cpheps I looked into it a bit closer since I got some test failures locally at some point (some tests will actually deadlock if they fail by not polling a log entry fast enough, which confused me), and yeah, it's just generally hard to test these things better with how the file input operator is right now.

A future goal might be refactoring a bit and have this better testable, and rewriting some of these tests so they aren't so reliant on timeouts.

cpheps
cpheps previously approved these changes Feb 21, 2022
@BinaryFissionGames BinaryFissionGames merged commit 5ff3f35 into master Feb 23, 2022
@BinaryFissionGames BinaryFissionGames deleted the try-reduce-flakiness branch February 25, 2022 02:46
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.

4 participants