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

Oblitirate test flakiness #756

Merged
merged 3 commits into from
May 25, 2022
Merged

Oblitirate test flakiness #756

merged 3 commits into from
May 25, 2022

Conversation

Wondertan
Copy link
Member

@Wondertan Wondertan commented May 24, 2022

Fixes #683
Fixes #467

Also, #736 did a good job for us and:
Closes #582
Closes #608
Hopefully closes #584
Hopefully closes #579

Bonus: #476
(it does not really fix this #476, but it fixes the flakeyness that was mixed up with #476, while #476 is another one that was not reproduced for a long time, and I assume it is now fixed)

@Wondertan Wondertan added area:core_and_app Relationship with Core node and Celestia-App kind:misc Attached to miscellaneous PRs labels May 24, 2022
@Wondertan Wondertan self-assigned this May 24, 2022
@Wondertan Wondertan changed the title tests(core): rewrite tests in core pkg to remove some of the falkeyness tests(core): rewrite tests in core pkg to remove some of the flakeyness May 24, 2022
@Wondertan Wondertan requested a review from adlerjohn May 24, 2022 17:40
@Wondertan Wondertan force-pushed the hlib/fix-core-falkeyness branch 3 times, most recently from 86af54d to 177c1d2 Compare May 24, 2022 18:00
@Wondertan
Copy link
Member Author

And to be honest. We are fixing tests and wasting time on tests with zero value to the code coverage.

@codecov-commenter
Copy link

codecov-commenter commented May 24, 2022

Codecov Report

Merging #756 (07114e4) into main (7997a44) will increase coverage by 0.18%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main     #756      +/-   ##
==========================================
+ Coverage   52.67%   52.85%   +0.18%     
==========================================
  Files         116      116              
  Lines        6574     6574              
==========================================
+ Hits         3463     3475      +12     
+ Misses       2748     2740       -8     
+ Partials      363      359       -4     
Impacted Files Coverage Δ
core/testing.go 83.14% <ø> (ø)
header/p2p/server.go 56.52% <0.00%> (+3.26%) ⬆️
fraud/bad_encoding.go 34.44% <0.00%> (+3.33%) ⬆️
header/core/listener.go 58.49% <0.00%> (+5.66%) ⬆️
header/verify.go 81.39% <0.00%> (+6.97%) ⬆️

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 7997a44...07114e4. Read the comment docs.

@Wondertan
Copy link
Member Author

I rerun tests 3 times already, and all of them pass

@Wondertan
Copy link
Member Author

Wondertan commented May 24, 2022

Forth run, one guy failed, fixing him

@Wondertan Wondertan force-pushed the hlib/fix-core-falkeyness branch 2 times, most recently from 2946a86 to 1e4db49 Compare May 24, 2022 18:51
@Wondertan
Copy link
Member Author

Ok, 3 more times, and it passes. Should be enough

Copy link
Member

@Bidon15 Bidon15 left a comment

Choose a reason for hiding this comment

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

🚢 🚢 🚢 🚢
yes yes yes yes

@Wondertan Wondertan changed the title tests(core): rewrite tests in core pkg to remove some of the flakeyness Oblitirate flakeyness May 24, 2022
@Wondertan Wondertan force-pushed the hlib/fix-core-falkeyness branch 2 times, most recently from a6aef54 to 42ff319 Compare May 24, 2022 19:16
Copy link
Member

@renaynay renaynay left a comment

Choose a reason for hiding this comment

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

MORGE IT

@Wondertan
Copy link
Member Author

image

image

@Wondertan
Copy link
Member Author

Ok, I accidentally pushed runtime.GOMAXPROCS(1), which fixed heightSub on CI completely, but it is cheating

@Wondertan
Copy link
Member Author

With some runtime tricks I was able tor reproduce and fix all the flakes, but this one is not and it makes me insane

@liamsi
Copy link
Member

liamsi commented May 24, 2022

ctx, cancel := context.WithTimeout(context.Background(), time.Second*5)

maybe try increasing the timeout there? I think if we rely on timeouts we will always run into some flakines no?

Copy link
Member

@liamsi liamsi left a comment

Choose a reason for hiding this comment

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

Changes look gud

@Wondertan
Copy link
Member Author

Wondertan commented May 25, 2022

The test does not fail because of the timeout. It fails because, for some reason, one test routine is executed after the background routine.

maybe try increasing the timeout there? I think if we rely on timeouts we will always run into some flakines no?

I put global timeouts in general for tests with concurrency. If they deadlock for some flakey or other reason.

@Wondertan Wondertan changed the title Oblitirate flakeyness Oblitirate test flakiyness May 25, 2022
@Wondertan Wondertan changed the title Oblitirate test flakiyness Oblitirate test flakiness May 25, 2022
@Wondertan
Copy link
Member Author

I think the heightSub test is fixed forever with the time sleep. However #579 showed up again lol

@Wondertan
Copy link
Member Author

I ran CI 6 times and on the seventh time, it failed with something I didn't see before.
image

@Wondertan
Copy link
Member Author

#759 But it is unrelated to this PR so I am merging this

@Wondertan Wondertan merged commit 84b7688 into main May 25, 2022
@Wondertan Wondertan deleted the hlib/fix-core-falkeyness branch May 25, 2022 11:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:core_and_app Relationship with Core node and Celestia-App kind:misc Attached to miscellaneous PRs
Projects
No open projects
Archived in project
5 participants