Skip to content

Conversation

@jongiddy
Copy link
Contributor

@jongiddy jongiddy commented Jan 13, 2026

The code currently has no tests for the data CRC. Add a test that the GzEncoders generate the CRC expected by RFC 1952 and that the GzDecoders fail if the CRC does not match.

This will give us more confidence to make any changes to the CRC calculation (e.g. #523 or #526).

Copy link
Member

@Byron Byron left a comment

Choose a reason for hiding this comment

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

Thanks a lot, I also think these will be very useful!

I'd wave the PR through, but then again, I have no clue. Maybe @folkertdev could take a look to see what he thinks might be missing. Based on this, we can decide if there should be more, or of that 'more' can or should be in another PR.
But maybe there are legitimate change requests as well, so I'd want to wait for his opinion.

Copy link
Contributor

@folkertdev folkertdev left a comment

Choose a reason for hiding this comment

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

Some minor style ideas/suggestions. Overall I think this gives good coverage of how flate2 uses checksums.

my PR additionally tests the public API (e.g. combine and reset). So together I think that would cover crc usage well.

@Byron
Copy link
Member

Byron commented Jan 15, 2026

Thanks a lot for chiming in @folkertdev! @jongiddy, I will wait until you had a chance to look at the suggestions, but generally will be happy to merge.

The code currently has no tests for the data CRC. Add a test that the GzEncoders
generate the CRC expected by RFC 1952 and that the GzDecoders fail if the CRC
does not match.
@jongiddy
Copy link
Contributor Author

Thanks for the great suggestions. I have applied them all.

Copy link
Member

@Byron Byron left a comment

Choose a reason for hiding this comment

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

Thanks a lot everyone! I'd merge this.

However, please allow one last question: could these tests be in tests/ as they don't use private APIs? It looks like it, and I would have checked (and possibly done) the move myself but then I can't merge it anymore 😅.

Then again, as this doesn't introduce tests, we could also move the tests over, if possible, in a separate PR and call this one done enough.

@Byron Byron merged commit f4924fe into rust-lang:main Jan 17, 2026
15 checks passed
@jongiddy
Copy link
Contributor Author

The tricky part of moving to tests is that the new tests uses the Rfc1952Crc type, which is also used by the roundtrip_header test which does use private APIs (GzBuilder::into_header).

@Byron
Copy link
Member

Byron commented Jan 17, 2026

Ah, I see, thanks for clarifying.

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