-
Notifications
You must be signed in to change notification settings - Fork 109
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
Tests: Add unit tests for org.osbuild.gunzip stage #1689
base: main
Are you sure you want to change the base?
Conversation
25a93e2
to
160c8c1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working on this! A quick first look with some ideas/suggestions
160c8c1
to
f25e9b9
Compare
@mvo5 thanks for the review, I have applied some changes to the code.
I mean - how to include |
f25e9b9
to
11cd7dd
Compare
11cd7dd
to
6797e7b
Compare
The here here seems to be from "test_file.txt.gz" vs "fake_file.txt.gz" not from the stdout? |
This PR is stale because it has been open 30 days with no activity. Remove "Stale" label or comment or this will be closed in 7 days. |
@elkoniu Hey, this got the "stale" label now, but all it needs to get merged is a tiny fix in the unit tests (see previous comment). would you mind fixing it so that this can get in? |
bf11787
to
97ff022
Compare
@mvo5 thanks for pointing this out, I have just updated the test case so it will not fail anymore :) |
3de4929
to
53db168
Compare
autopep8 is still unhappy: https://github.com/osbuild/osbuild/actions/runs/9061539700/job/24893504856?pr=1689#step:3:366 |
This PR is stale because it has been open 30 days with no activity. Remove "Stale" label or comment or this will be closed in 7 days. |
This PR was closed because it has been stalled for 30+7 days with no activity. |
@elkoniu the autopep8 tests are still unhappy, please see #1689 (comment) |
53db168
to
0f433f1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! This looks good, I still have some ideas and suggestions inline for your consideration. Hope they are useful.
# This test evaluates final binary call and match between its flags and stage args | ||
@mock.patch("subprocess.run") | ||
def test_gunzip_cmdline(mock_run, stage_module, test_input, expected): | ||
tmp_path = "/tmp" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(nitpick) I would not hardcode /tmp here, instead something like fake_tree = "/fake/output/tree"
or similar, first to make the mock scenario more realistic but also to avoid that this ever gets copied/pasted and used in "real" (non-mocked) scenarios where hardcoding /tmp will lead to (potenial) nasty issues (like races)
|
||
|
||
@pytest.mark.parametrize("test_input, expected", [ | ||
({"path": "fake_file.txt"}, ["/tmp/fake_file.txt.gz"]), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(nitpick) if there is only a single parameter for the test I would simply not parameterize but instead just put it into the test itself
fake_archive = f"{fake_file}.gz" | ||
fake_input_path = tmp_path / fake_file | ||
|
||
with fake_input_path.open("w") as fp: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure we need the md5sum and the 10mb file here, just decompressing a small file and comparing the content is probably good enough. With that the test can be written in a slightly more compact way maybe? Here is one possible idea:
# This test creates a compressed dummy file then decompresses and
# compares the content.
@pytest.mark.skipif(not has_executable("gunzip"), reason="gunzip")
def test_gunzip_integration(tmp_path, stage_module):
fake_archive_path = tmp_path / "some-file.gz"
fake_file_content = b"Some random text written to a file"
with gzip.GzipFile(fake_archive_path, "w") as fp:
fp.write(fake_file_content)
inp = {
"file": {
"path": tmp_path,
"data": {
"files": {
fake_archive_path.name: "Some test file"
}
},
},
}
expected_output_path = tmp_path / "tree" / "output.txt"
expected_output_path.parent.mkdir()
options = {
"path": expected_output_path.name
}
stage_module.main(inp, expected_output_path.parent, options)
assert expected_output_path.read_bytes() == fake_file_content
(needs a import gzip at the top too)
(full diff: pr1689.diff.txt)
wdyt?
This PR adds unit tests for org.osbuild.gunzip stage.