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 cases to test rohmu's handling zero size file #603

Closed
wants to merge 1 commit into from

Conversation

0xlianhu
Copy link
Contributor

@0xlianhu 0xlianhu commented Sep 27, 2023

Add test cases to test Rohmu handle zero size files:

Generate an empty file -> compress(lzma/zstd/snappy) and encrypt -> upload (local file system) -> download, decrypt+decompress -> Verify if download files are expected files

Moved this test case to Rohmu instead.

Aiven-Open/rohmu#156

@0xlianhu 0xlianhu force-pushed the 0xlianhu-add-rohmu-acceptance-tests branch 3 times, most recently from d8e9c76 to 0b6f90d Compare September 28, 2023 09:30
@0xlianhu 0xlianhu marked this pull request as draft September 28, 2023 09:33
@codecov-commenter
Copy link

codecov-commenter commented Sep 28, 2023

Codecov Report

Merging #603 (b7a4c9a) into main (6fee738) will decrease coverage by 0.03%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #603      +/-   ##
==========================================
- Coverage   91.30%   91.28%   -0.03%     
==========================================
  Files          32       32              
  Lines        4727     4727              
==========================================
- Hits         4316     4315       -1     
- Misses        411      412       +1     

see 2 files with indirect coverage changes

@0xlianhu 0xlianhu force-pushed the 0xlianhu-add-rohmu-acceptance-tests branch from 0b6f90d to b3dc8a8 Compare September 28, 2023 15:58
@0xlianhu 0xlianhu requested a review from packi September 29, 2023 06:39
@0xlianhu 0xlianhu marked this pull request as ready for review September 29, 2023 06:39
@0xlianhu 0xlianhu force-pushed the 0xlianhu-add-rohmu-acceptance-tests branch from b3dc8a8 to d12aef7 Compare September 29, 2023 07:09
@0xlianhu 0xlianhu changed the title pghoard,rohmu: add test cases to test rohmu's handling zero size file… Add test cases to test rohmu's handling zero size file Sep 29, 2023
@0xlianhu 0xlianhu force-pushed the 0xlianhu-add-rohmu-acceptance-tests branch 3 times, most recently from d66573c to 43011c0 Compare September 29, 2023 10:07
@0xlianhu 0xlianhu requested a review from Mulugruntz October 3, 2023 09:12
Copy link
Contributor

@Mulugruntz Mulugruntz left a comment

Choose a reason for hiding this comment

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

Please see the comments in thread.
Also, there are places that also apply but I didn't want to repeat the same comment in several places :).

test/test_rohmu.py Outdated Show resolved Hide resolved
test/test_rohmu.py Outdated Show resolved Hide resolved
test/test_rohmu.py Outdated Show resolved Hide resolved
test/test_rohmu.py Outdated Show resolved Hide resolved
test/test_rohmu.py Outdated Show resolved Hide resolved
test/test_rohmu.py Outdated Show resolved Hide resolved
test/test_rohmu.py Outdated Show resolved Hide resolved
test/test_rohmu.py Outdated Show resolved Hide resolved
test/test_rohmu.py Outdated Show resolved Hide resolved
test/test_rohmu.py Show resolved Hide resolved
@0xlianhu 0xlianhu force-pushed the 0xlianhu-add-rohmu-acceptance-tests branch 2 times, most recently from 77847f2 to 959588e Compare October 3, 2023 14:37
@0xlianhu
Copy link
Contributor Author

0xlianhu commented Oct 3, 2023

Please see the comments in thread. Also, there are places that also apply but I didn't want to repeat the same comment in several places :).

Thank you very much for your valuable comments. The file was updated according to your comments.

@0xlianhu 0xlianhu dismissed Mulugruntz’s stale review October 3, 2023 16:12

Comments addressed

@0xlianhu 0xlianhu requested a review from Mulugruntz October 3, 2023 16:13
test/test_rohmu.py Outdated Show resolved Hide resolved
test/test_rohmu.py Outdated Show resolved Hide resolved
Copy link
Contributor

@Mulugruntz Mulugruntz left a comment

Choose a reason for hiding this comment

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

This looks very good, @0xlianhu !

It seems you missed some of the previous comments (had to click on "expand"). I'll let you read them. The PR is good as it is already. Ping me if you want me to merge it as-is. Otherwise, that gives you some time to re-read.

@0xlianhu 0xlianhu force-pushed the 0xlianhu-add-rohmu-acceptance-tests branch from 959588e to a6389e3 Compare October 6, 2023 09:59
@0xlianhu 0xlianhu requested a review from Mulugruntz October 6, 2023 10:00
Mulugruntz
Mulugruntz previously approved these changes Oct 6, 2023
Copy link
Contributor

@Mulugruntz Mulugruntz left a comment

Choose a reason for hiding this comment

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

That's looking great! Less lines = usually a good thing :-).
:shipit:

test/test_rohmu.py Outdated Show resolved Hide resolved
test/test_rohmu.py Outdated Show resolved Hide resolved
@0xlianhu 0xlianhu force-pushed the 0xlianhu-add-rohmu-acceptance-tests branch from a6389e3 to 968306b Compare October 10, 2023 11:25
@0xlianhu 0xlianhu force-pushed the 0xlianhu-add-rohmu-acceptance-tests branch from 968306b to 0ceffa2 Compare October 18, 2023 14:00
@0xlianhu 0xlianhu requested a review from a team October 18, 2023 14:00
@0xlianhu 0xlianhu marked this pull request as draft October 18, 2023 14:13
@0xlianhu 0xlianhu force-pushed the 0xlianhu-add-rohmu-acceptance-tests branch from 0ceffa2 to 4952b35 Compare October 19, 2023 06:50
@0xlianhu 0xlianhu force-pushed the 0xlianhu-add-rohmu-acceptance-tests branch from 4952b35 to b7a4c9a Compare October 19, 2023 06:55
@0xlianhu 0xlianhu marked this pull request as ready for review October 19, 2023 09:33
@packi
Copy link
Contributor

packi commented Oct 31, 2023

The new test doesn't seem to include anything besides rohmu, so it's probably better suited to be in that code base?

@0xlianhu
Copy link
Contributor Author

Moving the PR to Rohmu repo instead as @packi suggested.
Aiven-Open/rohmu#156

@0xlianhu 0xlianhu closed this Nov 16, 2023
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.

4 participants