-
Notifications
You must be signed in to change notification settings - Fork 26
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
chore(sequencing): remove mock context from manager_test.rs #2524
base: main
Are you sure you want to change the base?
Conversation
859f44c
to
98fba66
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2524 +/- ##
==========================================
- Coverage 40.10% 9.72% -30.39%
==========================================
Files 26 87 +61
Lines 1895 10491 +8596
Branches 1895 10491 +8596
==========================================
+ Hits 760 1020 +260
- Misses 1100 9434 +8334
- Partials 35 37 +2 ☔ View full report in Codecov by Sentry. |
c39c276
to
c2f07b7
Compare
98fba66
to
786dfe2
Compare
c2f07b7
to
8364862
Compare
786dfe2
to
3e94c68
Compare
8364862
to
2b8cf29
Compare
3e94c68
to
8b53dad
Compare
2b8cf29
to
dcfa88f
Compare
8b53dad
to
22f54b1
Compare
dcfa88f
to
7afd08a
Compare
22f54b1
to
392ca7c
Compare
7afd08a
to
852e9f3
Compare
392ca7c
to
5873234
Compare
852e9f3
to
c08f77b
Compare
5873234
to
ffa5cb3
Compare
c08f77b
to
a49ea50
Compare
7037bab
to
bb55d27
Compare
b720f3f
to
61eae2c
Compare
bb55d27
to
b14dc7a
Compare
61eae2c
to
6a806b0
Compare
b14dc7a
to
c38d6ec
Compare
6a806b0
to
203fb99
Compare
c38d6ec
to
336697d
Compare
6d5e7bd
to
6a92643
Compare
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.
Reviewed 3 of 3 files at r2, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @matan-starkware)
6a92643
to
4a0bbd7
Compare
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.
Reviewed 3 of 3 files at r2, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @guy-starkware)
crates/sequencing/papyrus_consensus/src/test_utils.rs
line 24 at r2 (raw file):
Init(ProposalInit), Fin(ProposalFin), }
Do we actually need Fin to be part of this enum?
Code quote:
#[derive(Debug, PartialEq, Clone)]
pub enum TestProposalPart {
Init(ProposalInit),
Fin(ProposalFin),
}
crates/sequencing/papyrus_consensus/src/test_utils.rs
line 52 at r2 (raw file):
_ => panic!("Invalid TestProposalPart conversion"), }; <Vec<u8>>::try_from(init).expect("Invalid TestProposalPart conversion")
?
Suggestion:
init.try_into().expect("Invalid TestProposalPart conversion")
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.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @matan-starkware)
crates/sequencing/papyrus_consensus/src/test_utils.rs
line 24 at r2 (raw file):
Previously, matan-starkware wrote…
Do we actually need Fin to be part of this enum?
I guess we don't :)
crates/sequencing/papyrus_consensus/src/test_utils.rs
line 52 at r2 (raw file):
Previously, matan-starkware wrote…
?
Done.
4a0bbd7
to
385f3a6
Compare
I've removed the MockTestContext from manager_test.rs and instead mved it to depend on the mock context in test_utils.rs.