Automated nbdkit data plugin supports base64 in format strings#6842
Automated nbdkit data plugin supports base64 in format strings#6842xiaodwan merged 1 commit intoautotest:masterfrom
Conversation
WalkthroughAdds a new Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
350eab7 to
26e2f05
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@v2v/tests/src/nbdkit/nbdkit.py`:
- Around line 815-845: The test currently allows an invalid test_state and
doesn't assert the command exit status; add an explicit else branch that calls
test.fail for unknown test_state, and in the "positive" branch assert the
process.run exit code indicates success (e.g. result.exit_status == 0) before
checking that original_text is in result.stdout; in the "negative" branch assert
the command failed (e.g. result.exit_status != 0) before inspecting
result.stderr_text for err_msg (use params_get to get expected_err_msg) and call
test.fail with clear messages if expectations are not met; reference
variables/functions: test_state, cmd_str, process.run, result
(exit_status/stdout/stderr_text), original_text, invalid_b64, err_msg,
params_get, and test.fail.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6bc05cb2-c886-42d9-b377-026bdb414ff6
📒 Files selected for processing (2)
v2v/tests/cfg/nbdkit/nbdkit.cfgv2v/tests/src/nbdkit/nbdkit.py
|
@xiaodwan Please have a look. |
Signed-off-by: Ganesh Hubale <ghubale@redhat.com>
26e2f05 to
f2ac25f
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
v2v/tests/src/nbdkit/nbdkit.py (1)
865-891:⚠️ Potential issue | 🟠 MajorAdd exit status checks and handle invalid test_state.
The test function is missing critical assertions that were flagged in a previous review:
- Positive test (line 875): Should verify
result.exit_status == 0before checking stdout content, to ensure the command succeeded as expected.- Negative test (line 885): Should verify
result.exit_status != 0before checking stderr content, to ensure the command failed as expected.- Invalid test_state (after line 890): Should add an
elsebranch to fail explicitly iftest_stateis neither "positive" nor "negative".🛡️ Proposed fix with exit status checks and else branch
def test_data_plugin_supports_base64_in_format_string(): import base64 test_state = params_get(params, "test_state") if test_state == "positive": # Prepare the data original_text = b"nbdkit-test123" # Encode bytes to a base64 string b64_input = base64.b64encode(original_text).decode() cmd_str = f"nbdkit -U - data \'base64:{b64_input}\' --run 'nbdcopy $uri -'" # Execute command result = process.run(cmd_str, shell=True, ignore_status=True) + if result.exit_status != 0: + test.fail(f"nbdkit command failed unexpectedly: {result.stderr_text}") # Result.stdout contains the raw binary output from nbdcopy cmd_output = result.stdout if original_text not in cmd_output: test.fail(f"Base64 decoding failed. Expected {original_text!r}, got {cmd_output!r}") elif test_state == "negative": # Negative scenario - Check with invalid value invalid_b64 = "base64:SGVsbG8@@@" cmd_str = f"nbdkit -U - data \'{invalid_b64}\' --run 'nbdcopy $uri -'" # Execute command result = process.run(cmd_str, shell=True, ignore_status=True) + if result.exit_status == 0: + test.fail("Invalid base64 input unexpectedly succeeded.") # Verify the error message cmd_output = result.stderr_text err_msg = params_get(params, "expected_err_msg") if err_msg not in cmd_output: test.fail(f"Error message - \'{err_msg}\' not appeared.") + else: + test.error(f"Unsupported test_state: {test_state!r}")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@v2v/tests/src/nbdkit/nbdkit.py` around lines 865 - 891, The test_data_plugin_supports_base64_in_format_string function lacks exit-status assertions and an invalid-state branch; update the positive branch to assert result.exit_status == 0 before inspecting result.stdout, update the negative branch to assert result.exit_status != 0 before inspecting result.stderr_text, and add an else branch that calls test.fail(...) if test_state is neither "positive" nor "negative" so the test fails loudly for unknown states.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@v2v/tests/src/nbdkit/nbdkit.py`:
- Around line 865-891: The test_data_plugin_supports_base64_in_format_string
function lacks exit-status assertions and an invalid-state branch; update the
positive branch to assert result.exit_status == 0 before inspecting
result.stdout, update the negative branch to assert result.exit_status != 0
before inspecting result.stderr_text, and add an else branch that calls
test.fail(...) if test_state is neither "positive" nor "negative" so the test
fails loudly for unknown states.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 194e5b49-5e07-4e00-8eeb-7699296b0d04
📒 Files selected for processing (2)
v2v/tests/cfg/nbdkit/nbdkit.cfgv2v/tests/src/nbdkit/nbdkit.py
🚧 Files skipped from review as they are similar to previous changes (1)
- v2v/tests/cfg/nbdkit/nbdkit.cfg
Test results:
Summary by CodeRabbit