Skip to content

tests: skip flaky node tests#4862

Merged
algorandskiy merged 2 commits intoalgorand:masterfrom
Eric-Warehime:remove-flaky-node-tests
Dec 8, 2022
Merged

tests: skip flaky node tests#4862
algorandskiy merged 2 commits intoalgorand:masterfrom
Eric-Warehime:remove-flaky-node-tests

Conversation

@Eric-Warehime
Copy link
Copy Markdown
Contributor

Summary

#4824 Re-enabled 3 tests in node_tests.go which have since caused nightly build failures every night. I've gone ahead and deleted the two tests I see failing every night--they haven't actually been run for years, and some of this code dates back to the original commit.

@Eric-Warehime Eric-Warehime added tech debt Things that need re-work for simplification / sanitization to reduce implementation overhead Skip-Release-Notes labels Dec 5, 2022
@codecov
Copy link
Copy Markdown

codecov Bot commented Dec 5, 2022

Codecov Report

Merging #4862 (abaf0d8) into master (93c01b5) will decrease coverage by 0.00%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #4862      +/-   ##
==========================================
- Coverage   54.03%   54.03%   -0.01%     
==========================================
  Files         427      427              
  Lines       53474    53474              
==========================================
- Hits        28897    28896       -1     
- Misses      22312    22313       +1     
  Partials     2265     2265              
Impacted Files Coverage Δ
crypto/merkletrie/trie.go 66.42% <0.00%> (-2.19%) ⬇️
crypto/merkletrie/node.go 91.62% <0.00%> (-1.87%) ⬇️
ledger/blockqueue.go 85.63% <0.00%> (-1.73%) ⬇️
catchup/service.go 69.80% <0.00%> (+0.48%) ⬆️
ledger/testing/randomAccounts.go 57.23% <0.00%> (+0.61%) ⬆️
data/transactions/verify/txn.go 74.57% <0.00%> (+0.84%) ⬆️
ledger/roundlru.go 96.22% <0.00%> (+5.66%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

winder
winder previously approved these changes Dec 6, 2022
@algorandskiy
Copy link
Copy Markdown
Contributor

I suggest to disable them back and make another approach later

@Eric-Warehime
Copy link
Copy Markdown
Contributor Author

I suggest to disable them back and make another approach later

These tests have been disabled since the initial commit of the project https://github.com/algorand/go-algorand/blame/381a8352020d2a043631944b3fe58e3c76e377dc/node/node_test.go#L181

I'm fine with disabling them if you want, but unless I personally get time to look at them it doesn't seem like anyone else is going to.

Also, I think having tests here that don't run gives somewhat of a false sense of having coverage where you don't (even though the coverage report reflects that they don't run).

@algonautshant
Copy link
Copy Markdown
Contributor

I suggest we set a timeline for deleting these tests. In 6 months, if we cannot collaboratively resolve the issues, then we delete them. First, please add to this PR the information about the failures of the tests, and let the reviewers evaluate why they are failing.

@Eric-Warehime
Copy link
Copy Markdown
Contributor Author

Test failures can be seen in nightly tests https://app.circleci.com/pipelines/github/algorand/go-algorand/10983/workflows/c9fcb7ba-7be3-4217-80e4-ae8b5baa0a9b/jobs/188091
They consistently passed locally and I never saw the non-nightly CI fail so I'm not really able to root cause them effectively. The errors are hard to diagnose without going into a debugger--the tests spin up a bunch of nodes and try to sync data between them.

@algorandskiy algorandskiy merged commit 829a3c9 into algorand:master Dec 8, 2022
@algorandskiy algorandskiy changed the title tests: Remove flaky node tests tests: skip flaky node tests Dec 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Skip-Release-Notes tech debt Things that need re-work for simplification / sanitization to reduce implementation overhead

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants