-
Notifications
You must be signed in to change notification settings - Fork 82
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
[TEST] use EXPECT_RANGE_EQ in get_test... #2445
Conversation
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/seqan/seqan3/6zKCgq7smcCbM5oMpXQU9mgb97Zf |
Codecov Report
@@ Coverage Diff @@
## master #2445 +/- ##
==========================================
- Coverage 98.26% 98.26% -0.01%
==========================================
Files 267 267
Lines 11065 11064 -1
==========================================
- Hits 10873 10872 -1
Misses 192 192
Continue to review full report at Codecov.
|
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 very good!
However, I noticed that the tests use a lot of meaningless variables like cmp0
, t
and is
. Maybe you want to change something about that?
And I have a question about the using literal stuff. Maybe you want to add a short documentation notice?
I approve either way, you can decide if you want to rename the variables or if that is too much effort.
test/unit/range/views/get_test.cpp
Outdated
std::vector<seqan3::phred42> cmp1{seqan3::phred42{0}, seqan3::phred42{1}, seqan3::phred42{2}, seqan3::phred42{3}}; | ||
std::vector<seqan3::phred42> functor1 = seqan3::views::get<1>(t) | seqan3::views::to<std::vector>; | ||
EXPECT_EQ(cmp1, functor1); | ||
EXPECT_RANGE_EQ(cmp0, seqan3::views::get<0>(t)); |
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.
As there is no cmp00
anymore you could rename cmp0
to cmp
or maybe maybe use a more explanatory name as functor_compared
.
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.
I renamed the stuff in the get test.
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.
Open point about renaming, otherwise LGTM
😭 I don't want to rename everything. |
You don't necessarily need to, but you should address the point :) |
Part of seqan/product_backlog#83.