chore(common): Remove LinkedChunkBuilderTest
#4695
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4695 +/- ##
=======================================
Coverage 85.90% 85.90%
=======================================
Files 292 292
Lines 33909 33852 -57
=======================================
- Hits 29129 29082 -47
+ Misses 4780 4770 -10 ☔ View full report in Codecov by Sentry. |
| // Chunk with no next chunk comes last. | ||
| chunks.sort_by(|a, b| b.next.cmp(&a.next)); | ||
|
|
||
| let last_chunk = chunks.pop().unwrap(); |
There was a problem hiding this comment.
I know that this is meant to be a test function, but it isn't marked as cfg(test) and those unneeded unwraps bother me.
Why not replace the is_empty() check with a:
let Some(last_chunk) = vec.last() else {
return Ok(None)
};There was a problem hiding this comment.
It's not behind a cfg(test) because it's used by other crates :-/.
I can do a pop() else yup.
There was a problem hiding this comment.
Actually the pop cannot fail because chunks is guaranteed to not be empty. I am adding a // SAFETY comment, and I am replacing unwrap by expect.
| let chunk_identifier_generator = | ||
| ChunkIdentifierGenerator::new_from_previous_chunk_identifier(last_chunk_identifier); | ||
|
|
||
| let mut linked_chunk = from_last_chunk(Some(last_chunk), chunk_identifier_generator)?.unwrap(); |
There was a problem hiding this comment.
Not sure if you can get rid of this one, but at least put an expect() there instead.
There was a problem hiding this comment.
Sure, let's add an expect. For the record, it's purely used for testing purposes, but I share your concerns.
There was a problem hiding this comment.
I've changed for a else { return Ok(None); } here.
This patch adds the new `from_all_chunks` function in the `linked_chunk::lazy_loader` module. It is only used for testing purposes. It aims at replacing `LinkedChunkBuilderTest` (see next patches). Why? Because `from_all_chunks` uses `from_last_chunk` and `insert_new_first_chunk`: if `from_all_chunks` is able to find all errors that `LinkedChunkBuilderTest` finds, it's a bingo. Transitively, it proves that `from_last_chunk` and `insert_new_first_chunk` are correct!
This patch replaces all usages of `LinkedChunkBuilderTest` by `from_all_chunks`.
This patch removes `LinkedChunkBuilderTest` and updates tests accordingly.
Hywan
force-pushed
the
chore-common-remove-linked-chunk-builder-test-part-2
branch
from
130f5ff to
181bd84
Compare
This patch adds the new
from_all_chunksfunction in thelinked_chunk::lazy_loadermodule. It is only used for testingpurposes. It aims at replacing
LinkedChunkBuilderTest.Why? Because
from_all_chunksusesfrom_last_chunkandinsert_new_first_chunk: iffrom_all_chunksis able to find allerrors that
LinkedChunkBuilderTestfinds, it's a bingo. Transitively,it proves that
from_last_chunkandinsert_new_first_chunkarecorrect!
The other patches replaces
LinkedChunkBuilderTestand remove it.EventCachestorage #3280EventCachelazy-loading #4632