Skip to content
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

Add typed test to kmer_hash #1722

Merged
merged 2 commits into from
May 5, 2020
Merged

Conversation

MitraDarja
Copy link
Contributor

@MitraDarja MitraDarja commented Apr 7, 2020

Add a typed_test to kmer_hash to reduce code lines, while testing more. Found and solves the bug #1719.

@codecov
Copy link

codecov bot commented Apr 14, 2020

Codecov Report

Merging #1722 into master will decrease coverage by 0.04%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1722      +/-   ##
==========================================
- Coverage   97.66%   97.62%   -0.05%     
==========================================
  Files         239      239              
  Lines        9089     9062      -27     
==========================================
- Hits         8877     8847      -30     
- Misses        212      215       +3     
Impacted Files Coverage Δ
include/seqan3/range/views/kmer_hash.hpp 100.00% <100.00%> (ø)
include/seqan3/range/views/take_until.hpp 92.85% <0.00%> (-5.26%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 74fb213...8cd8edd. Read the comment docs.

@MitraDarja MitraDarja requested review from a team and marehr and removed request for a team April 14, 2020 15:09
@MitraDarja
Copy link
Contributor Author

@marehr When you review, can you also take a look at the code coverage test failure? I don't understand, why the coverage drops in take_until... I am using it as it was used before.

@MitraDarja
Copy link
Contributor Author

@marehr polite ping

Copy link
Member

@marehr marehr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the late review. I must somehow marked it as read in https://github.com/notifications.

include/seqan3/range/views/kmer_hash.hpp Outdated Show resolved Hide resolved
test/unit/range/views/view_kmer_hash_test.cpp Outdated Show resolved Hide resolved
test/unit/range/views/view_kmer_hash_test.cpp Outdated Show resolved Hide resolved
test/unit/range/views/view_kmer_hash_test.cpp Outdated Show resolved Hide resolved
test/unit/range/views/view_kmer_hash_test.cpp Outdated Show resolved Hide resolved
test/unit/range/views/view_kmer_hash_test.cpp Outdated Show resolved Hide resolved
test/unit/range/views/view_kmer_hash_test.cpp Outdated Show resolved Hide resolved
test/unit/range/views/view_kmer_hash_test.cpp Outdated Show resolved Hide resolved
test/unit/range/views/view_kmer_hash_test.cpp Outdated Show resolved Hide resolved
test/unit/range/views/view_kmer_hash_test.cpp Outdated Show resolved Hide resolved
@MitraDarja
Copy link
Contributor Author

@marehr I have now made all tests typed tests, only the tests based on previous issues are not typed tests. I am not sure, if it makes sense for the issue_tests to be typed as well.

I have not made an extra test for testing the combined_with_other_views when using the stop_at_thymine view, because that would have added only one test case for gapped and ungapped, while adding more lines. So I put them in the combined_with_container tests for a shorter code, I hope that is okay.

Copy link
Member

@marehr marehr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just some reminders for me; We should meet up and talk this over in Person :)

include/seqan3/range/views/kmer_hash.hpp Outdated Show resolved Hide resolved
test/unit/range/views/view_kmer_hash_test.cpp Outdated Show resolved Hide resolved
test/unit/range/views/view_kmer_hash_test.cpp Outdated Show resolved Hide resolved
test/unit/range/views/view_kmer_hash_test.cpp Outdated Show resolved Hide resolved
result_t gapped4{gapped2};
static constexpr auto ungapped_view = seqan3::views::kmer_hash(seqan3::ungapped{3});
static constexpr auto gapped_view = seqan3::views::kmer_hash(0b101_shape);
static constexpr auto prefix_until_first_thymine = seqan3::views::take_until([] (seqan3::dna4 x)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe put that into the type test class kmer_hash_properties_test

Copy link
Member

@marehr marehr May 2, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You wrote me that you couldn't make this work. Since these are view adaptors, it is fine for me to put them here.

I'm wondering why TestFixture::ungapped_view wasn't working.

Copy link
Member

@marehr marehr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks fine;

@eseiler eseiler requested a review from a team May 3, 2020 11:12
Copy link
Member

@smehringer smehringer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only two small things.

Also, since this PR does two things if I understand correctly (Fixing a bug and restructuring the test) can you rebase your changes into 2 commits, one for the restructuring and one for the bug?

CHANGELOG.md Outdated Show resolved Hide resolved
include/seqan3/range/views/kmer_hash.hpp Show resolved Hide resolved
@MitraDarja MitraDarja force-pushed the kmer_hash_typed_test branch 2 times, most recently from abed13c to d12333c Compare May 4, 2020 10:13
@MitraDarja MitraDarja requested a review from smehringer May 4, 2020 11:38
CHANGELOG.md Outdated Show resolved Hide resolved
@MitraDarja MitraDarja force-pushed the kmer_hash_typed_test branch 2 times, most recently from 2d14c6e to 81c330e Compare May 4, 2020 13:57
@MitraDarja MitraDarja requested a review from smehringer May 4, 2020 15:19
Copy link
Member

@smehringer smehringer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can rebase on the current master now and use the EXPECT_RANGE_EQ (see #1764 for examples) and then the Jenkins error should go away.
Then we can merge this.

@smehringer smehringer merged commit e9db871 into seqan:master May 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants