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

rewrite tests #163

Merged
merged 4 commits into from
Nov 10, 2021
Merged

rewrite tests #163

merged 4 commits into from
Nov 10, 2021

Conversation

figsoda
Copy link
Member

@figsoda figsoda commented Nov 4, 2021

rewrite most tests with proptest, tests single file {,de}compression and directory {,de}compression

Also found an issue with chaining lz4 compressions: tar.lz4.lz4 files are not being compressed correctly, so it is ignored for now

I don't think file permissions are working on windows and for zip files, so I disabled checking them in these situations

Closes #30
Closes #31
Closes #50

@figsoda

This comment has been minimized.

@marcospb19
Copy link
Member

Will give a proper review in a couple days. Got a bit busy again.

@marcospb19 marcospb19 added the bugfix Fixes an existing bug label Nov 4, 2021
@figsoda
Copy link
Member Author

figsoda commented Nov 6, 2021

resolved merge conflicts

@GabrielSimonetto
Copy link
Collaborator

GabrielSimonetto commented Nov 7, 2021

I really like this addition, but the scary part for me, as a non-initiated proptest user, is if we will be able to interpret the tests and be sure that they are doing their job along time.

Also, we probably should write something about it on CONTRIBUTING.md as to how to work with it... there should be some stuff to write about on it, but the first thing that comes to mind is just a reference to the proptest book, but also, how we should treat the proptest_regressions dir, (which, the book seems to say that it should be added to git registry? idk).

I will keep looking into it so as to help understand how we should work with proptest, but you can help guide us on how to make this workflow more understandable that would be neat (:

p.s.: I will add that I believe this crate and philosophy of testing is amazing and I would find it really fun and useful to get some experience with it, even if it requires some work

@GabrielSimonetto
Copy link
Collaborator

Btw, have you understood how these changes fixed #50 ? It would be nice to know what we were doing wrong, for future reference

@figsoda
Copy link
Member Author

figsoda commented Nov 7, 2021

Added some more information to CONTRIBUTING.md

I have no idea how this fixes #50. I actually gave up investigating #50 and just bundled with a couple other issues and rewrote the tests.

Copy link
Member

@vrmiguel vrmiguel left a comment

Choose a reason for hiding this comment

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

Very cool :D

I think that the assert_cmd approach is very interesting but it could bite us in the future if we ever decide to add code coverage metrics, I guess

@marcospb19
Copy link
Member

I have no idea how this fixes #50. I actually gave up investigating #50 and just bundled with a couple other issues and rewrote the tests.

That's a mystery, I wish anyone knew whats causing it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix Fixes an existing bug
Projects
None yet
5 participants