Skip to content

test(core): add unit tests for has-facet-values promotion condition#4882

Open
GabrielRoc wants to merge 3 commits into
vendurehq:masterfrom
GabrielRoc:test/has-facet-values-condition
Open

test(core): add unit tests for has-facet-values promotion condition#4882
GabrielRoc wants to merge 3 commits into
vendurehq:masterfrom
GabrielRoc:test/has-facet-values-condition

Conversation

@GabrielRoc

@GabrielRoc GabrielRoc commented Jun 27, 2026

Copy link
Copy Markdown
Contributor

Description

Adds a direct unit test suite for the at_least_n_with_facets promotion condition (has-facet-values-condition.ts), which previously had 0% function coverage.

The condition delegates per-line facet matching to FacetValueChecker and sums the qualifying quantities against minimum. Because FacetValueChecker is a NestJS-injected service, the test injects a vi.fn() mock via the condition's init() hook — no database or TestServer is required.

7 tests covering:

  • Boundary-value analysis on total matching quantity vs minimum (below / at / above)
  • Correct aggregation across multiple lines
  • Correct exclusion of non-matching lines
  • Verification that ctx and the facet ID list are forwarded as-is to hasFacetValues()

Breaking changes

None.

Checklist

  • I have set a clear title
  • My PR is small and contains a single feature
  • I have added or updated test cases

Relates to #4835


View with Codesmith Autofix with Codesmith
Need help on this PR? Tag /codesmith with what you need. Autofix is disabled.

Adds a spec file covering the `at_least_n_with_facets` check() via
equivalence partitioning and boundary-value analysis. Uses a lightweight
mock injected via init() to avoid database access. 7 tests.

Relates to vendurehq#4835
Copilot AI review requested due to automatic review settings June 27, 2026 04:51
@vercel

vercel Bot commented Jun 27, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
vendure-storybook Ready Ready Preview, Comment Jul 6, 2026 12:14am

Request Review

@vendure-ci-automation-bot

vendure-ci-automation-bot Bot commented Jun 27, 2026

Copy link
Copy Markdown
Contributor

All contributors have signed the CLA ✍️ ✅
Posted by the CLA Assistant Lite bot.

@coderabbitai

coderabbitai Bot commented Jun 27, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 2abcab68-7521-4f2e-830e-fcb930a81f4c

📥 Commits

Reviewing files that changed from the base of the PR and between ba13e06 and 8da9b63.

📒 Files selected for processing (1)
  • packages/core/src/config/promotion/conditions/has-facet-values-condition.spec.ts
✅ Files skipped from review due to trivial changes (1)
  • packages/core/src/config/promotion/conditions/has-facet-values-condition.spec.ts

📝 Walkthrough

Walkthrough

Adds a Vitest spec for hasFacetValues that initializes the condition with a mocked FacetValueChecker.hasFacetValues, builds test order data with a request context, and verifies minimum-threshold behavior, accumulation across matching lines, exclusion of non-matching lines, and the arguments passed to the checker.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title is concise and accurately describes the main change: adding unit tests for the has-facet-values promotion condition.
Description check ✅ Passed The description includes a summary, breaking changes, checklist, and related issue; the optional screenshots section is the only omission.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

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 a focused Vitest unit test suite for the at_least_n_with_facets (hasFacetValues) promotion condition in @vendure/core, improving direct function coverage of this business-logic condition without requiring DB/TestServer setup.

Changes:

  • Introduces a new spec file covering threshold/boundary behavior for minimum vs summed matching quantities.
  • Adds multi-line aggregation and non-matching exclusion scenarios.
  • Verifies FacetValueChecker.hasFacetValues() is invoked with the expected (line, facets, ctx) arguments via an injected mock.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +58 to +62
const ctx = createRequestContext({ pricesIncludeTax: false });

async function check(order: Order, args: ConfigArg[]) {
return hasFacetValues.check(ctx, order, args, undefined as any);
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Dismissed. hasFacetValues.check() does not use the promotion argument. Passing undefined as any is the intentional, consistent pattern across all promotion-condition specs in this PR series.

Comment on lines +126 to +134
it('passes the ctx and facet ID list to hasFacetValues', async () => {
const order = orderWith([line('v1', 1)]);
const args = buildArgs(1, ['f1', 'f2']);
mockHasFacetValues.mockResolvedValue(true);

await check(order, args);

expect(mockHasFacetValues).toHaveBeenCalledWith(order.lines[0], ['f1', 'f2'], ctx);
});

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Valid point. Fixed in commit 8da9b635c: added mockHasFacetValues.mockClear() at the start of the test to reset the call history, and added expect(mockHasFacetValues).toHaveBeenCalledTimes(1) to pin the assertion to exactly one invocation from this test. The toHaveBeenCalledWith assertion is now unambiguous even if more tests are added later.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In
`@packages/core/src/config/promotion/conditions/has-facet-values-condition.spec.ts`:
- Around line 25-28: The mock in has-facet-values-condition.spec.ts is using the
old two-parameter vi.fn generic form, which is incompatible with Vitest 3.x.
Update the mock declaration for mockHasFacetValues to use the current single
function-type generic signature based on FacetValueChecker['hasFacetValues'], so
the test type-checks correctly with the newer vi.fn API.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: b11cadd5-a9db-40ab-b6ce-43959280bbec

📥 Commits

Reviewing files that changed from the base of the PR and between c5a2785 and ba13e06.

📒 Files selected for processing (1)
  • packages/core/src/config/promotion/conditions/has-facet-values-condition.spec.ts

@GabrielRoc

Copy link
Copy Markdown
Contributor Author

I have read the CLA Document and I hereby sign the CLA

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants