-
Notifications
You must be signed in to change notification settings - Fork 30
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
Minor test fixes #3256
Minor test fixes #3256
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Comment is more of a follow up than an issue with this change
let mut metadata = TestDescription::default_more_nodes(); | ||
metadata.overall_safety_properties.num_failed_views = 6; | ||
metadata.overall_safety_properties.num_failed_views = 5; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm this one is a bit weird. It should be 6 because there are 6 dead nodes. I'm guessing we don't count one of the view sync views as a failure or something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, I don't understand this myself.
we only get 5 failed views, and it seemed to consistently be the same 5 on all networks: Results: [NotEnoughFailures { expected: 6, failed_views: {ViewNumber(14), ViewNumber(18), ViewNumber(15), ViewNumber(20), ViewNumber(17)}]
Views 16 and 19 don't count as failed, for whatever reason -- actually, this was exactly the motivation for me wanting to change the way we counted failed_views
to use QC
s instead of/in addition to decides.
* bump (#3291) * flake.lock: Update (#3294) Flake lock file updates: • Updated input 'fenix': 'github:nix-community/fenix/9a025daf6799e3af80b677f0af57ef76432c3fcf?narHash=sha256-ih8NPk3Jn5EAILOGQZ%2BKS5NLmu6QmwohJX%2B36MaTAQE%3D' (2024-06-01) → 'github:nix-community/fenix/ab655c627777ab5f9964652fe23bbb1dfbd687a8?narHash=sha256-ixopuTeTouxqTxfMuzs6IaRttbT8JqRW5C9Q/57WxQw%3D' (2024-06-08) • Updated input 'fenix/rust-analyzer-src': 'github:rust-lang/rust-analyzer/d6d735e6f20ef78b16a79886fe28bd69cf059504?narHash=sha256-qBruki5NHrSqIw5ulxtwFmVsb6W/aOKOMjsCJjfalA4%3D' (2024-05-31) → 'github:rust-lang/rust-analyzer/48bbdd6a74f3176987d5c809894ac33957000d19?narHash=sha256-%2BlRAmz92CNUxorqWusgJbL9VE1eKCnQQojglRemzwkw%3D' (2024-06-05) • Updated input 'nixpkgs': 'github:NixOS/nixpkgs/6132b0f6e344ce2fe34fc051b72fb46e34f668e0?narHash=sha256-7R2ZvOnvd9h8fDd65p0JnB7wXfUvreox3xFdYWd1BnY%3D' (2024-05-30) → 'github:NixOS/nixpkgs/d226935fd75012939397c83f6c385e4d6d832288?narHash=sha256-HV97wqUQv9wvptiHCb3Y0/YH0lJ60uZ8FYfEOIzYEqI%3D' (2024-06-07) Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> * Bump clap from 4.5.6 to 4.5.7 (#3302) * Bump url from 2.5.0 to 2.5.1 (#3304) * Fix Leaf Ascension (#3307) * fix tests, halfway improve impl * move the last of the variables * fix upgrade tests * move to helper method * lint * fix last suite * use debugged function, remove dead tests * fix tests * fix lint * comments, fix last test * Minor test fixes (#3256) * Revert "Minor test fixes (#3256)" (#3316) This reverts commit 1932e42. * Docs improvements (#3295) * Update General.md * Update QuorumProposalRecv.md * Update Timeout.md * Update ViewSyncTrigger.md * Update VoteOnQuorumProposal.md * [DEPENDENCY_REFACTOR] - Remove proposal validation from the quorum proposal task (#3275) * Move validation * Fix fmt and import * Fix build and recv test * Remove a todo * Restore an event * Fix non-dependency lints * Fix tests * Fix CI * Fix fmt and imports * Fix proposal and proposal recv tasks tests * 3 more tests * Fix doc * Address comments * Add error * Fix Conficts in keyao/remove-proposal-validation (#3308) * fix * remove dead test * fix lint * fix test * Fix vote tests --------- Co-authored-by: Keyao Shen <[email protected]> * Fix upgrade test --------- Co-authored-by: Jarred Parr <[email protected]> * Fee upgrade (#3236) * Multiple builders (#3203) * Multiple builders support * Bump vec1 from 1.12.0 to 1.12.1 (#3319) * Bump libp2p-identity from 0.2.8 to 0.2.9 (#3318) * [DEPENDENCY_REFACTOR] - Modify `update_validated_state_map` to not override a view with more info (#3315) * Move validation * Fix fmt and import * Fix build and recv test * Remove a todo * Restore an event * Fix non-dependency lints * Fix tests * Fix CI * Fix fmt and imports * Fix proposal and proposal recv tasks tests * 3 more tests * Fix doc * Address comments * Add error * Fix Conficts in keyao/remove-proposal-validation (#3308) * fix * remove dead test * fix lint * fix test * Fix vote tests --------- Co-authored-by: Keyao Shen <[email protected]> * Save changes * Fix upgrade test * Fix build * Add error handling to update_validated_state_map * Fix tests * use ref --------- Co-authored-by: Jarred Parr <[email protected]> * Choose builder url on orchestrator * Multiple builders * Fix lints * Fix malformed URLs --------- Co-authored-by: Jarred Parr <[email protected]> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: ss-es <[email protected]> Co-authored-by: iwantanode <[email protected]> Co-authored-by: Keyao Shen <[email protected]>
No linked issue.
This PR:
Several tests had
num_failed_views
set far too generously, which allowed them to pass when they should not have. This PR fixes that, and causes integration tests to fail we do not fail exactlynum_failed_views
views.This PR does not:
Key places to review: