Skip to content

feat(coverage): bidmodifiers add/set typed fixtures (#137, #183)#185

Merged
axisrow merged 1 commit into
mainfrom
feat/coverage-bidmodifiers-fixtures-137
May 20, 2026
Merged

feat(coverage): bidmodifiers add/set typed fixtures (#137, #183)#185
axisrow merged 1 commit into
mainfrom
feat/coverage-bidmodifiers-fixtures-137

Conversation

@axisrow

@axisrow axisrow commented May 20, 2026 •
edited
Loading

Copy link
Copy Markdown
Owner

Fixes #137. Fixes #183 (bucket A — bidmodifiers).

Summary

Verification

  • test_dry_run_payload_schema_coverage / test_all_canonical_dry_run_commands_have_payload_coverage_or_exclusion pass.
  • build_api_coverage_report.py: schema_parity_ok=true, nested_schema_violations=[], operation_waiver_misuse=[].

Copilot AI review requested due to automatic review settings May 20, 2026 01:46
@axisrow axisrow added this to the 0.3.8 milestone May 20, 2026
Copilot AI reviewed May 20, 2026
View reviewed changes

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Adds schema-gate payload fixtures for bidmodifiers.add and bidmodifiers.set so they are covered by the generic dry-run payload coverage tests (instead of being excluded), aligning bidmodifiers with the typed-fixture coverage strategy used across other mutating commands.

Changes:

  • Added PAYLOAD_CASES fixtures for bidmodifiers add (MOBILE_ADJUSTMENT) and bidmodifiers set (Id + value shape).
  • Removed bidmodifiers.add and bidmodifiers.set from DRY_RUN_PAYLOAD_EXCLUSIONS while keeping bidmodifiers.delete excluded per existing rationale.

@axisrow @claude
feat(coverage): bidmodifiers add/set typed fixtures (#137, #183)
7b3ce45 
- Add PAYLOAD_CASES for bidmodifiers.add (Mobile adjustment) and bidmodifiers.set.
- Remove corresponding entries from DRY_RUN_PAYLOAD_EXCLUSIONS.
- bidmodifiers.delete kept excluded per #136 (Helper/legacy surface).

Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
@axisrow
axisrow force-pushed the feat/coverage-bidmodifiers-fixtures-137 branch from 89546bc to 7b3ce45 Compare May 20, 2026 03:18
@axisrow

axisrow commented May 20, 2026

Copy link
Copy Markdown
Owner Author

@claude review. Focus on critical issues: bugs, security vulnerabilities, logical errors, data loss risks, performance problems. Do NOT nitpick style, naming conventions, minor formatting, or subjective preferences — only flag issues that could break functionality or cause real harm in production.

@claude

claude Bot commented May 20, 2026 •
edited
Loading

Copy link
Copy Markdown

Claude finished @axisrow's task in 2m 59s —— View job

Claude Code Review

  • Read PR diff
  • Inspect bidmodifiers command + payload schema rules
  • Check for critical issues (bugs, schema mismatches, regressions)
  • Post review

Verdict: no critical issues. Fixtures are minimal, correctly shaped against the WSDL, and use the right code paths. Safe to merge.

Verification

Walked both fixtures through direct_cli/commands/bidmodifiers.py against tests/wsdl_cache/bidmodifiers.xml:

bidmodifiers add --campaign-id 12 --type MOBILE_ADJUSTMENT --value 120 → produces:

{"method": "add", "params": {"BidModifiers": [{"MobileAdjustment": {"BidModifier": 120}, "CampaignId": 12}]}}
  • Routes through the non-plural branch (MOBILE_ADJUSTMENT ∉ _PLURAL_NESTED_KEYS), no special-case required flags triggered — bidmodifiers.py:193-234 ✓
  • Matches BidModifierAddItem (extends BidModifierAddBase for CampaignId/AdGroupId; all adjustment subobjects minOccurs="0") — tests/wsdl_cache/bidmodifiers.xml:320-342 ✓

bidmodifiers set --id 10 --value 150 → produces:

{"method": "set", "params": {"BidModifiers": [{"Id": 10, "BidModifier": 150}]}}
  • Takes the correct modifier_id branch (not the legacy broken --campaign-id/--type shape) — bidmodifiers.py:317-319 ✓
  • Matches BidModifierSetItem which requires Id + BidModifier (both minOccurs="1") — tests/wsdl_cache/bidmodifiers.xml:343-348 ✓

bidmodifiers.delete correctly remains in DRY_RUN_PAYLOAD_EXCLUSIONS per the PR description.

One observation (non-blocking)

The schema gate in _assert_body_matches_wsdl only enforces min_occurs > 0 fields (test_api_coverage.py:231-275). Since every adjustment subobject in BidModifierAddItem has minOccurs="0", the gate cannot enforce the "exactly one adjustment subobject" rule that the live API requires. The fixture itself is fine (it sends MobileAdjustment), but anyone adding future bidmodifiers.add variants should be aware the schema gate alone won't catch a missing adjustment object — runtime tests are still the safety net.

No security, performance, or data-loss concerns. The diff is purely test-fixture additions and registry bookkeeping.
· branch feat/coverage-bidmodifiers-fixtures-137

@axisrow
axisrow merged commit 403a50d into main May 20, 2026
6 checks passed
@axisrow
axisrow deleted the feat/coverage-bidmodifiers-fixtures-137 branch May 20, 2026 03:29
This was referenced May 20, 2026
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot Copilot left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

0.3.8

2 participants

@axisrow