refactor(test): deduplicate consumers controller test setup

[ravverma]

Summary

• Extract shared NonAdminConsumersController into beforeEach, eliminating 6 identical inline esmock blocks (one per describe suite) that were repeated verbatim
• Collapse 3 structurally identical immutable-field tests (clientId, technicalAccountId, imsOrgId) into a single forEach loop
• Remove the defensive pathInfo=undefined test — covered by the simpler "header absent" test and no longer required under the 90% coverage threshold

Test plan

• npm test passes with all 10,185 tests green and no coverage threshold errors
• npx mocha test/controllers/consumers.test.js shows 61 passing

🤖 Generated with Claude Code

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

[solaris007]

Hey @ravverma,

Strengths

• Clean DRY pass: the new shared NonAdminConsumersController collapses six verbatim inline esmock blocks into a single beforeEach extraction (test/controllers/consumers.test.js:151-162), and the forEach over the immutable-field set (lines 761-770) folds three structurally identical tests into one parameterized run. Net -153 lines with no observable behavior change.
• The shared consumerModelMock and slackMock (test/controllers/consumers.test.js:113-138) are now a single source of truth for both admin and non-admin variants. Future Consumer enum additions or Slack mock changes require one edit, not seven.
• Behavioral equivalence on the non-admin tests is preserved. The production controller in src/controllers/consumers.js returns forbidden(...) before any Consumer.X or Slack call fires, so the shared mocks never reach the runtime paths the inline ad-hoc mocks previously stubbed. The substitution is sound.
• forEach test names render as three distinct mocha tests (rejects immutable field clientId, ... technicalAccountId, ... imsOrgId) via the template at line 762, so failure output stays diagnosable.
• Sandbox lifecycle is preserved correctly. createSandbox runs in beforeEach (line 87), sandbox.restore() runs in afterEach (line 166). No state leakage between admin and non-admin controller instances - each test gets a fresh sandbox and fresh esmocked modules.
• Test fixtures continue to use clearly-marked dummy values (test-client-id, test-ta-id, valid-ta-token, test-ims-org@AdobeOrg). No real credentials, IMS tokens, or org IDs introduced.
• Test count math checks out: the file's it() calls plus the forEach expansion produce 61 runtime tests, matching the PR description.
• CI is green and Codecov reports all modified lines covered.

Issues

Minor (Nice to Have)

• forEach loop dropped the 'immutable' substring assertion at test/controllers/consumers.test.js:761-770. Pre-PR, the clientId test asserted that x-error included BOTH 'immutable' AND 'clientId'; the technicalAccountId and imsOrgId tests only checked the field name. The new uniform loop only checks the field name. Combined with the existing rejects multiple immutable fields at once test (lines 771-786), which also only asserts on field names, the post-PR file contains zero 'immutable' assertions. If a future refactor rewords the production error message at src/controllers/consumers.js (currently "The following fields are immutable and cannot be updated: ...") to something like "Cannot update protected fields...", no test in this file would fail. Fix: Add expect(response.headers.get('x-error')).to.include('immutable') inside the forEach body. One line.

• package-lock.json peer: true removals are unrelated to the PR's stated scope. 28 lines of metadata cleanup across helix-universal, langchain, openai, react, @aws-sdk/client-dynamodb, eslint, and others. The changes are mechanically a no-op (no version, integrity, license, or dependency-tree changes) - almost certainly an incidental npm relayout from a local npm install. Mixing unrelated changes erodes the description-to-diff trust contract and makes future git bisect noisier. Fix: Either factor the lockfile changes into a separate chore(deps): refresh lockfile peer flags PR, or extend the PR description to acknowledge them as incidental cleanup. Given the no-op nature, the latter is acceptable.

• Removed pathInfo=undefined test leaves a branch-coverage gap on the defensive optional chain. The PR description is correct that the kept "header absent" test (test/controllers/consumers.test.js:433-446) covers the same observable behavior, but the two tests traverse different branches of const headers = context.pathInfo?.headers || {}; in src/controllers/consumers.js:

  • Removed test (pathInfo: undefined): the ?. short-circuits, the || {} fallback fires.
  • Kept test (pathInfo: { headers: {} }): ?. evaluates to {} (truthy), the || does NOT fire.

  Same status code, same error string, different branches. The defensive ?. is no longer exercised by any test in the suite. A future "cleanup" that drops the ?. would not be caught. Fix: Either restore the 12-line test, or add a one-line comment in the production code explaining the defensive intent so a future refactor does not strip the ?. thinking it is gratuitous. Author's call - both are defensible.

Recommendations

• Consider hoisting both esmocks to before instead of beforeEach (test/controllers/consumers.test.js:140-162). The two esmock calls only depend on static constants and a fixed boolean, not on per-test state - the per-test context is passed to the controller invocation, not to esmock. Hoisting would cut esmock invocations from roughly 122 (2 per test across 61 tests) to 2 total. The current shape is correct; this is a runtime touch-up.
• A brief comment on the beforeEach block explaining that NonAdminConsumersController is intentionally identical to ConsumersController except for the access-control mock would shave a few seconds off future reader comprehension when they see the apparent duplication.

Out of scope, worth tracking

• The consumers controller's access-control wiring imports AccessControlUtil statically, which is why this test file needs esmock for access-control variation. Adjacent files in this folder (e.g., test/controllers/entitlements.test.js) configure access-control via a per-suite sinon stub and avoid esmock entirely. Aligning the consumers controller's wiring with the dominant pattern would let this test file drop esmock for access-control variation entirely. Separate PR.
• The production register/update/revoke flows interpolate user-controlled consumerName into Slack-bound strings without escaping embedded backticks. Admin-only endpoint, internal channel, low real-world severity, but a follow-up could add a sanitizer for Slack-bound interpolations.

Assessment

Ready to merge? Yes.

Reasoning: A tidy, well-scoped test refactor with no production-code surface. All six "non-admin returns forbidden" assertions are preserved across the affected methods. The three Minor findings are quality observations, not blockers. The most actionable item before merge is either dropping the unrelated package-lock.json peer-flag changes or noting them in the PR description so future archaeologists can separate signal from noise.

Next Steps

1. Optional one-line addition: expect(response.headers.get('x-error')).to.include('immutable') inside the forEach loop.
2. Either factor out or document the package-lock.json peer-flag cleanup in the PR description.
3. Optional: restore the pathInfo=undefined test, or add a defensive-intent comment in production.

The PR is mergeable as-is from a code, security, architecture, and QA standpoint.