Skip to content

fix: relax over-strict discount applied[] count/position assertions - #74

Open
XiaolongZhang-TT wants to merge 1 commit into
Universal-Commerce-Protocol:mainfrom
XiaolongZhang-TT:fix/relax-discount-applied-count-assertions
Open

fix: relax over-strict discount applied[] count/position assertions#74
XiaolongZhang-TT wants to merge 1 commit into
Universal-Commerce-Protocol:mainfrom
XiaolongZhang-TT:fix/relax-discount-applied-count-assertions

Conversation

@XiaolongZhang-TT

Copy link
Copy Markdown
Contributor

Description

Two discount tests in business_logic_test.py asserted exact lengths (and one a fixed position) on discounts.applied[], but per discount.md the applied list carries code-based discounts plus any automatic discounts, whose count is not fixed and whose code is null. A conformant business that also applies an automatic discount would fail these assertions even though every submitted code is correctly present.

test_multiple_discounts_accepted

self.assertTrue(discounts_obj and len(discounts_obj.applied) == 2)   # too strict
applied_codes = [d.code for d in discounts_obj.applied]
self.assertIn(valid_code_1, applied_codes)
self.assertIn(valid_code_2, applied_codes)

The two assertIn(code) checks already prove both submitted codes were applied, so the len == 2 adds nothing except a failure mode when an automatic discount coexists. Relaxed to assertTrue(discounts_obj.applied).

test_multiple_discounts_one_rejected

self.assertTrue(discounts_obj and len(discounts_obj.applied) == 1)   # too strict
self.assertEqual(discounts_obj.applied[0].code, valid_code)          # assumes position

Two issues: the exact count, and applied[0] assuming the code-based discount is first — an automatic discount ordered ahead of it has code = null and fails the equality. Replaced with a position-independent membership check.

assertIn is safe against a list that may contain None entries (automatic discounts), since the submitted code is always a string.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected, including removal of schema files or fields)
  • Documentation update

Is this a Breaking Change or Removal?

N/A — test-only fix, no schema/field removal.

Checklist

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation (N/A — test-only fix)
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

Two discount tests asserted exact lengths and a fixed position on
discounts.applied[], but discount.md states applied carries code-based
discounts *plus any automatic discounts*, whose count is not fixed and
whose code is null. A conformant business that also applies an automatic
discount would fail these assertions even though every submitted code is
present.

- test_multiple_discounts_accepted: drop `len(applied) == 2`; the two
  assertIn(code) checks already prove both codes applied.
- test_multiple_discounts_one_rejected: drop `len(applied) == 1` and the
  `applied[0].code == valid_code` position assumption; assert membership
  instead, tolerant of automatic discounts ordered first.

assertIn is safe against a list that may contain None entries from
automatic discounts, since the submitted code is always a string.
@damaz91 damaz91 added the status:needs-triage Signal that the PR is ready for human triage label Aug 7, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

status:needs-triage Signal that the PR is ready for human triage

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants