Repository navigation
Conversation
test_user_settings swallowed the results of set_user_setting and delete_user_setting with `let _ = ...`, and hid the only assertion behind `if let Ok(s) = ...`. All three failure modes let the test report green while doing nothing: a wrong URL/body silently errors out and is discarded, or get_user_setting fails and the `if let` just skips the block. PyMISP's reference implementation (pinned e7debb4, pymisp/api.py:3700-3749) sends `setting` (and optional `user_id`) in the POST body to userSettings/getSetting, userSettings/setSetting, and userSettings/delete respectively — not as URL path segments. This client's set_user_setting/get_user_setting/delete_user_setting (src/client.rs:2512-2555) build the setting name into the request path instead, which is the separately tracked defect (F02-F04). This commit only fixes the test's vacuous assertions per finding A01; it does not touch the client's request-shape bug. Now set/get/delete are unwrap/expect'd and the round-trip assertion is unconditional, so the test fails loudly instead of passing vacuously. Note: this #[ignore]'d integration test needs a live MISP instance and cannot be run here; it will only pass once F02-F04 are fixed to match MISP's actual wire protocol. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JoSButbVaWEtGMXwTx73BZ
elhoim
marked this pull request as draft
September 3, 2026 05:04
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
BLUF —
test_user_settingsno longer passes with zero assertions executed.tests/integration_tests.rs,test_user_settingsdiscarded errors fromset_user_settinganddelete_user_settingwithlet _ =, and gated its only assertion behindif let Ok(s), so the test reported green even when nothing worked..expect(...)and theget_user_settingresult is checked unconditionally, assertings.setting == "dashboard_access".ubuntu-22.04has no php8.3 mod_php;test_event_crud_lifecycleraced the publish worker), fixed in unmerged PR fix: [ci] Run integration tests on ubuntu-24.04 to match php8.3 #53; not caused by this PR.Finding
test_user_settings can pass with zero assertions executed
set_user_setting and delete_user_setting are both let _ = (errors swallowed) and the sole assertion sits behind if let Ok(s). With F02-F04 broken, the test reports green while nothing works.
Change
test_user_settings previously used
let _ = ...on set_user_setting and delete_user_setting, discarding any errors, and gated the sole assertion behindif let Ok(s) = ...so a failing get_user_setting silently skipped the check entirely — the test could pass with zero real assertions executed. Changed all three calls to.expect(...)and made the get_user_setting result unconditional, assertings.setting == "dashboard_access"unconditionally. Verified the client method signatures (src/client.rs:2512-2555) all returnMispResult<...>and MispUserSetting::setting is a plain String (src/models/user_setting.rs), so the new.expect/assert_eq!type-checks without needing to compile (build/test are off-limits per ground rules).Upstream evidence
PyMISP pymisp/api.py:3700-3749 (pinned e7debb4f0427dbc33800dc565d771ef1535f3c2f) — get_user_setting POSTs to
userSettings/getSettingwith body{"setting": ..., "user_id": ...}; set_user_setting POSTs touserSettings/setSettingwith body{"setting": ..., "value": ..., "user_id": ...}; delete_user_setting POSTs touserSettings/deletewith body{"setting": ..., "user_id": ...}. This client's implementation (src/client.rs:2512-2555) instead buildssettinginto the URL path (e.g.userSettings/view/{setting},userSettings/setSetting/{setting}), which is a separate, already-tracked defect (referenced in the finding as F02-F04) and out of scope for this A01 fix — I did not touch src/client.rs.Verified against MISP core
843b6744and PyMISPe7debb4f.Tests
No new/runnable test added: the fix IS to tests/integration_tests.rs::test_user_settings itself, which is #[ignore]'d and requires a live MISP instance. It cannot be run in this environment (no live MISP, and cargo test/build is disallowed by ground rules). The test will only pass once the F02-F04 URL/body-shape defects in src/client.rs are fixed to match MISP's real wire protocol per the PyMISP citations above.Note on the original finding
Accurate as written. Confirmed both
let _ = ...swallows (set_user_setting, delete_user_setting) and theif let Ok(s) = ...guard hiding the sole assertion, exactly as described — a failure anywhere in the sequence would let the test report green with zero assertions actually executed.Caveats
PR body must state: these strengthened assertions will only pass once F02-F04 (the client's userSettings URL/body-shape bugs, see upstream_evidence) are fixed — until then this test will fail loudly against a live MISP instead of passing vacuously, which is the intended improvement. Could not run cargo test/build here (ground rules + no live MISP); verified type-compatibility of the edit by manual inspection of the method signatures and MispUserSetting struct instead.
Verified locally:
cargo fmt --check,cargo clippy --all-targets --all-features -- -D warnings,cargo test --all-features --lib,--test integration_tests --no-run, andcargo check --examples, each in an isolated target directory.🤖 Generated with Claude Code