-
Notifications
You must be signed in to change notification settings - Fork 11
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
refactor: rename test util #426
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #426 +/- ##
=======================================
Coverage 83.60% 83.60%
=======================================
Files 37 37
Lines 1720 1720
Branches 1720 1720
=======================================
Hits 1438 1438
Misses 205 205
Partials 77 77 ☔ View full report in Codecov by Sentry. |
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 1 of 1 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @ayeletstarkware and @giladchase)
crates/mempool/src/mempool_test.rs
line 58 at r1 (raw file):
// transactions. #[track_caller] fn check_mempool_queue_eq(mempool: &Mempool, expected_queue: &[ThinTransaction]) {
Suggestion:
verify_mempool_queue
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, 1 unresolved discussion (waiting on @ayeletstarkware and @elintul)
crates/mempool/src/mempool_test.rs
line 58 at r1 (raw file):
// transactions. #[track_caller] fn check_mempool_queue_eq(mempool: &Mempool, expected_queue: &[ThinTransaction]) {
Do you want us to use verify_ as a consistent naming in such test utils?
I think we should have a consistent naming scheme for test_utils that only assert things, for readability.
So readers will feel about them something similar to how they feel when they see assert_eq.
2af4855
to
fe7e1cd
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 all commit messages.
Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @ayeletstarkware and @giladchase)
crates/mempool/src/mempool_test.rs
line 58 at r1 (raw file):
Previously, giladchase wrote…
Do you want us to use verify_ as a consistent naming in such test utils?
I think we should have a consistent naming scheme for test_utils that only assert things, for readability.
So readers will feel about them something similar to how they feel when they see assert_eq.
Don't you think passing the expected argument makes it clear we're comparing?
I'm okay with a convention, but let's go with Rust conventions (something like assert_eq_mempool_queue
).
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 1 of 1 files at r2.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @ayeletstarkware and @giladchase)
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 all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @giladchase)
fe7e1cd
to
31880a6
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.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @elintul)
crates/mempool/src/mempool_test.rs
line 58 at r1 (raw file):
Don't you think passing the expected argument makes it clear we're comparing?
Here yes, but in PR 3 in the stack I want to check two things, then assert_eq will look like i'm comparing them to each other 🤔?
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, 1 unresolved discussion (waiting on @giladchase)
crates/mempool/src/mempool_test.rs
line 58 at r1 (raw file):
Previously, giladchase wrote…
Don't you think passing the expected argument makes it clear we're comparing?
Here yes, but in PR 3 in the stack I want to check two things, then assert_eq will look like i'm comparing them to each other 🤔?
Let's go with my suggestion, which sticks to Rust naming.
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, 1 unresolved discussion (waiting on @giladchase)
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, 1 unresolved discussion (waiting on @elintul)
crates/mempool/src/mempool_test.rs
line 58 at r1 (raw file):
Previously, elintul (Elin) wrote…
Let's go with my suggestion, which sticks to Rust naming.
You mean change this and PR 3 to assert_*
?
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, 1 unresolved discussion (waiting on @giladchase)
crates/mempool/src/mempool_test.rs
line 58 at r1 (raw file):
Previously, giladchase wrote…
You mean change this and PR 3 to
assert_*
?
Yes, assert_eq_the_thing_we_assert_on
.
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, 1 unresolved discussion (waiting on @giladchase)
crates/mempool/src/mempool_test.rs
line 58 at r1 (raw file):
Previously, elintul (Elin) wrote…
Yes,
assert_eq_the_thing_we_assert_on
.
Also, let's write this convention down in #elinpool. 😬
31880a6
to
461562b
Compare
It was a misnomer, we're asserting the queue, not the entire pool. commit-id:181fcd98
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 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @giladchase)
461562b
to
53de2b0
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.
Reviewable status: 0 of 1 files reviewed, all discussions resolved (waiting on @elintul)
crates/mempool/src/mempool_test.rs
line 58 at r1 (raw file):
Previously, elintul (Elin) wrote…
Also, let's write this convention down in #elinpool. 😬
Done
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 1 of 1 files at r4, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @giladchase)
It was a misnomer, we're asserting the queue, not the entire pool.
commit-id:181fcd98
Stack:
This change is