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

Seekable format empty string #3346

Merged

Conversation

daniellerozenblit
Copy link
Contributor

@daniellerozenblit daniellerozenblit commented Dec 13, 2022

This PR is a followup to a previous PR by @yhoogstrate. The goal of the previous PR was to fix a small bug in seekable_format that led to the omission of the ZSTD MAGIC when compressing an empty string.

I have validated the bug fix with the test case provided, but have created this PR to fix some minor issues (forgetting to free malloc'd memory in the test case).

@terrelln
Copy link
Contributor

This looks good to me once CI passes!

Only question is do you know if the seekable format tests run in CI?

@Cyan4973
Copy link
Contributor

Cyan4973 commented Dec 13, 2022

Only question is do you know if the seekable format tests run in CI?

Good point,
Seekable format tests can be triggered directly,
or as part of contrib/ tests,
which itself can also be triggered as part make test-all.

But ... looking at the list of tests on Github Actions,
it's unclear to me if any of these tests are run.
I'm sure they used to be part of CI,
but it might have been in the TravisCI era,
and they could have been forgotten in the transition towards Github Actions ...

edit : I can't find any trace of contrib/ tests within .github/ workflows,
so we actually might have to add this test.

edit 2: Actually, make contrib is also part of make all,
and we do have one make all test,
and it runs the seekable format tests :
https://github.com/facebook/zstd/actions/runs/3688718449/jobs/6243837843#step:3:228

So that's fine !

@yhoogstrate
Copy link
Contributor

wow thanks @daniellerozenblit to carry this forwards ! Good to see the test works and the contrib tests are included in the CI. Sorry I missed the free after malloc 😳

@daniellerozenblit daniellerozenblit marked this pull request as ready for review December 14, 2022 15:02
@daniellerozenblit daniellerozenblit requested review from Cyan4973 and terrelln and removed request for Cyan4973 and terrelln December 14, 2022 19:22
@daniellerozenblit daniellerozenblit merged commit 72845eb into facebook:dev Dec 14, 2022
@daniellerozenblit daniellerozenblit deleted the seekable-format-empty-string branch December 15, 2022 21:06
@Cyan4973 Cyan4973 mentioned this pull request Feb 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants