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 test to check if chained encoders finishes #4

Merged

Conversation

marcospb19
Copy link
Contributor

No description provided.

@marcospb19
Copy link
Contributor Author

marcospb19 commented Jun 11, 2023

Hey, I do believe this test should pass, but it hangs indefinitely.

The test chains 5 Encoders and writes on the top one, the result should be random data in the format .bz3.bz3.bz3.bz3.bz3, but it runs forever.

If you edit the chain number to 1, it succeeds immediately, try running:

cargo test --release -- chain

This is the smaller test I saw failing while I was doing a bigger one with encoder and decoders.

Idk, it's technically possible that bzip3 makes assumptions about buffers and I can't chain them like this.

@marcospb19
Copy link
Contributor Author

Related to #1, in Ouch it's possible to chain any number of formats when compressing or decompressing a file/archive.

@bczhc
Copy link
Owner

bczhc commented Jun 12, 2023

Good catch! This is a bug! Thanks for finding it. I'm on the way fixing them... For the tests, I hope {read, write}::{Bz3Encoder, Bz3Decoder} with a variety of block_size can all be covered. Now your 5 chained encoder test can pass. I'm working to fix and test another three cases.

@bczhc
Copy link
Owner

bczhc commented Jun 12, 2023

e2ed199. all chained cases seem to be OK. I use an eof flag to solve this, just not coming up with a more elegant approach...

@marcospb19 marcospb19 changed the title Add test to check if chained encoders finish Add test to check if chained encoders finishes Jun 12, 2023
@marcospb19
Copy link
Contributor Author

For reference, here is the original test I wrote, it chains write decoders with read encoders.

Do you want to commit the tests yourself? Otherwise I can edit this PR and try merging one of the two tests I've shown you, Whatever you prefer, let me know 🙏.

(Your responses are lightning quick ⚡⚡⚡, no need to rush though!)

@marcospb19
Copy link
Contributor Author

marcospb19 commented Jun 12, 2023

Oh, note that I set the random-data length to be bigger than the block size, that's to force Bz3Encoders to make multiple reads, if length was less than block size, I believe a single read for each item in the chain would be enough, but I wanted to test the "interleaving" calls of Read between the encoders, going back and forth.

I used:

block size = 70kb
random data length = 200kb

One thing I just realized is that only the first one might be forced to write it all, because compressed data is shrunk down.

200KB -> ReadEncoder1 (reads 200KB) -> ReadEncoder2 (reads X) -> ReadEncoder3 (reads Y)

If X and Y are below 70kb, then it's a single-block read, so we might have to do some tests and raise the random data length.

Maybe having two different tests... one with smaller input for single-block processing, and one with bigger input for multi-block processing.

@bczhc
Copy link
Owner

bczhc commented Jun 12, 2023 via email

@marcospb19 marcospb19 force-pushed the joao/test-chained-encoders-and-decoders branch from 25c1eb4 to 2a2a179 Compare July 11, 2023 17:36
@marcospb19 marcospb19 marked this pull request as ready for review July 11, 2023 17:36
@marcospb19
Copy link
Contributor Author

Hey, I'm back from a long gaming break 🤓🎮 , can you re-review?

Test passes since your last fix.

for single-block and multiple-block encoding
@marcospb19 marcospb19 force-pushed the joao/test-chained-encoders-and-decoders branch from 2a2a179 to b61aa21 Compare July 11, 2023 17:38
@marcospb19
Copy link
Contributor Author

Oops forgot to rebase.

@bczhc
Copy link
Owner

bczhc commented Jul 12, 2023

Oh great! LGTM, thanks!

@bczhc bczhc merged commit 9ae7798 into bczhc:master Jul 12, 2023
1 check passed
@marcospb19 marcospb19 deleted the joao/test-chained-encoders-and-decoders branch July 12, 2023 15:17
@marcospb19
Copy link
Contributor Author

marcospb19 commented Jul 12, 2023

With this merged I think I can go back to the Ouch side, I was getting a bug with chained formats and I think that was the cause.

I'll keep you updated on my progress in the other issue.

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.

2 participants