[FIX] compression streams writing wrong format #2458
Conversation
eseiler
requested review from
a team and
simonsasse
and removed request for
a team
|
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/seqan/seqan3/2LHgxV53uxvANUxuLoUpzk9ebJmK |
Codecov Report
@@ Coverage Diff @@
## master #2458 +/- ##
=======================================
Coverage 98.30% 98.30%
=======================================
Files 267 267
Lines 11063 11065 +2
=======================================
+ Hits 10875 10877 +2
Misses 188 188
Continue to review full report at Codecov.
|
test/unit/io/detail/misc_test.cpp
Outdated
| @@ -70,3 +74,52 @@ TEST(misc, valid_compression_extensions) | |||
| EXPECT_TRUE(std::find(valid_compression.begin(), valid_compression.end(), "zst") != valid_compression.end()); | |||
| #endif | |||
| } | |||
|
|
|||
| template <typename compression_t> | |||
There was a problem hiding this comment.
I personally would find it better, if this would be in a new test test/unit/io/detail/misc_output_test.cpp
test/unit/io/detail/misc_test.cpp
Outdated
| @@ -70,3 +74,52 @@ TEST(misc, valid_compression_extensions) | |||
| EXPECT_TRUE(std::find(valid_compression.begin(), valid_compression.end(), "zst") != valid_compression.end()); | |||
| #endif | |||
| } | |||
There was a problem hiding this comment.
the above seem like they belong to test/unit/io/detail/misc_input_test.cpp
There was a problem hiding this comment.
I left it in misc_test (didnt rename it), because it does not use misc_input.hpp
test/unit/io/detail/misc_test.cpp
Outdated
| using compression_types = ::testing::Types<seqan3::detail::gz_compression, | ||
| seqan3::detail::bgzf_compression, | ||
| seqan3::detail::bz2_compression>; |
There was a problem hiding this comment.
Can you double-check whether all of this compiles if gz, bgzf and bz2 aren't defined?
There was a problem hiding this comment.
It did not but now it does
test/unit/io/detail/misc_test.cpp
Outdated
| EXPECT_TRUE(seqan3::detail::starts_with(magic_header, this->expected_magic_header)); | ||
| EXPECT_FALSE(seqan3::detail::bgzf_compression::validate_header(std::span{magic_header})); | ||
| } | ||
| } |
There was a problem hiding this comment.
I think it would be easier to understand if this would be 3 test cases instead of 1 typed test as they have something in common, but are also quite different.
I mean the original problem was only gzip <=> bgzf
There was a problem hiding this comment.
Yes, it's better when it's split.
I did some more convoluted stuff before, and it did save a lot of code to have a fixture.
Then I managed to simplify everything and did not re-evaluate the usage of a fixture
test/unit/io/detail/misc_test.cpp
Outdated
|
|
||
| std::ifstream filestream{this->file_path()}; | ||
| std::array<char, this->biggest_magic_header_size> magic_header{}; | ||
| std::copy_n(std::istreambuf_iterator{filestream}, this->biggest_magic_header_size, magic_header.begin()); |
There was a problem hiding this comment.
Wouldn't something along the lines
std::vector<char> file_content{std::istreambuf_iterator{filestream}, std::istreambuf_iterator{}};easier to understand?
There was a problem hiding this comment.
Yup.
I needed to fix some stuff in our span implementation, mainly also stripping cv qualifiers in addition to ref when checking concepts and the deduction guide for ranges.
eseiler
force-pushed
the
fix/gz_bgzf
branch
2 times, most recently
from
c3cb612 to
d2071a9
Compare
* [DOC] Add changelog entry for #2458 * Update CHANGELOG.md Co-authored-by: Lydia Buntrock <[email protected]> Co-authored-by: Lydia Buntrock <[email protected]>
Documentation will be follow-up PR