Supersede #910: entry shape test with lint unblock#919
Supersede #910: entry shape test with lint unblock#919
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughA new test case is added to Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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. Comment |
Greptile SummaryThis PR adds a regression test for entry shape type narrowing and addresses unrelated lint failures by updating Key Changes:
The test ensures that Confidence Score: 5/5
Important Files Changed
Last reviewed commit: aed85d8 |
justin808
force-pushed
the
codex/supersede-910-entry-shape-lint
branch
from
aed85d8 to
6011002
Compare
justin808
added
codex
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Misleading assertion: duplicate asymmetric matchers match same element
- Replaced the duplicate asymmetric matcher with a single
expect.any(String)so the assertion no longer implies a two-element guarantee it does not enforce.
- Replaced the duplicate asymmetric matcher with a single
Or push these changes by commenting:
@cursor push 2811bbf22c
Preview (2811bbf22c)
diff --git a/test/package/environments/base.test.js b/test/package/environments/base.test.js
--- a/test/package/environments/base.test.js
+++ b/test/package/environments/base.test.js
@@ -60,7 +60,7 @@
expect(typeof baseConfig.entry.application).toBe("string")
expect(Array.isArray(baseConfig.entry.multi_entry)).toBe(true)
expect(baseConfig.entry.multi_entry).toStrictEqual(
- expect.arrayContaining([expect.any(String), expect.any(String)])
+ expect.arrayContaining([expect.any(String)])
)
})![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
ReviewThe PR is low-risk (test-only change), but there are two issues worth addressing. Bug: weak arrayContaining assertion (line 63-65)
Redundancy Both new type-shape assertions largely duplicate what the adjacent tests already verify more strictly. Suggestion If the intent is to explicitly document TypeScript-narrowing semantics (single-entry → string, multi-entry → string[]), that is a reasonable goal, but the test should be precise enough to actually fail when those shapes break. Consider tightening to check |
ReviewOverall: Low-risk addition of a regression test. No production code changes. A couple of issues worth addressing. Redundant assertions in stringEntries.forEachSee inline comment. The forEach loop that re-checks typeof entryValue === "string" is a no-op — those values were already selected by that exact predicate. It should be removed to keep signal-to-noise high. Simplification opportunityThe test can be made more direct by iterating once instead of building two filtered arrays. This expresses the intent directly — every entry must be either a non-empty string[] or a string — without the intermediate filter step that makes redundant assertions possible. .prettierignore workaroundThe comment reads "Temporarily ignore workflow files that cannot be modified in PRs due to claude-review validation". If these files have formatting issues, silencing prettier permanently is the wrong resolution. The "temporarily" qualifier should come with a tracking issue or a follow-up TODO referencing when/how the underlying formatting will actually be fixed, otherwise this will quietly become permanent. |
ReviewGood addition — a regression test protecting entry-value shapes is a clear win for TypeScript consumers of the config. Two issues worth addressing before merge: 1. 2. String entries are never validated (inline comment on lines 62–69) Minor: PR description mentions a Otherwise the test structure and intent are clear. |
| entryValues.length | ||
| ) | ||
| expect(stringEntries.length).toBeGreaterThan(0) | ||
| expect(arrayEntries.length).toBeGreaterThan(0) | ||
| expect(stringEntries.every((entryValue) => entryValue.length > 0)).toBe( | ||
| true | ||
| ) | ||
| expect(flattenedArrayEntries.length).toBeGreaterThan(0) | ||
| expect( | ||
| flattenedArrayEntries.every( | ||
| (entryValue) => typeof entryValue === "string" | ||
| ) | ||
| ).toBe(true) | ||
| }) |
There was a problem hiding this comment.
The .every(...).toBe(true) pattern gives very poor failure messages — when a value fails the predicate, Jest just says expected false to be true with no indication of which value failed or what it was.
Prefer individual assertions per entry using forEach, or at minimum use toStrictEqual on a mapped result:
| entryValues.length | |
| ) | |
| expect(stringEntries.length).toBeGreaterThan(0) | |
| expect(arrayEntries.length).toBeGreaterThan(0) | |
| expect(stringEntries.every((entryValue) => entryValue.length > 0)).toBe( | |
| true | |
| ) | |
| expect(flattenedArrayEntries.length).toBeGreaterThan(0) | |
| expect( | |
| flattenedArrayEntries.every( | |
| (entryValue) => typeof entryValue === "string" | |
| ) | |
| ).toBe(true) | |
| }) | |
| expect(stringEntries.length + arrayEntries.length).toBe( | |
| entryValues.length | |
| ) | |
| expect(stringEntries.length).toBeGreaterThan(0) | |
| expect(arrayEntries.length).toBeGreaterThan(0) | |
| stringEntries.forEach((entryValue) => | |
| expect(typeof entryValue).toBe("string") | |
| ) | |
| expect(flattenedArrayEntries.length).toBeGreaterThan(0) | |
| flattenedArrayEntries.forEach((entryValue) => | |
| expect(typeof entryValue).toBe("string") | |
| ) |
This way, a failure names the offending value directly.
| expect(arrayEntries.length).toBeGreaterThan(0) | ||
| expect(stringEntries.every((entryValue) => entryValue.length > 0)).toBe( |
There was a problem hiding this comment.
This assertion makes the test fragile. It requires the fixture data to always contain both single-file entries (strings) and multi-file entries (arrays) simultaneously. If a future fixture change removes either shape, the test fails for the wrong reason — it's testing the fixture's contents, not the production logic.
Consider either:
- Removing these two
toBeGreaterThan(0)guards and letting theforEachassertions below be the actual invariant (they'll trivially pass over an empty array, which is fine), or - Pinning this to known fixture entry names (
application→ string,multi_entry→ array) as the adjacent tests already do, which is explicit about what's being verified.
Review: PR #919 — Entry shape test + prettierignoreOverall: Low-risk addition (test-only, no production changes). Two issues worth addressing before merge. Issues1. Poor failure diagnostics ( The two 2. Fragile fixture dependency
.prettierignore changeAdding workflow files to |
1 participant
Supersedes #910.
Includes:
Closes #792
Note
Low Risk
Adds a regression test around
baseConfig.entryvalue types; no runtime/production code changes. Risk is limited to potential test brittleness if entry generation behavior intentionally changes.Overview
Adds a new Jest regression test in
test/package/environments/base.test.jsto ensurebaseConfig.entrypreserves stable value shapes: single-entry points remainstrings and multi-file entry points remainstring[](supporting TypeScript narrowing).No production logic changes; this PR only tightens test coverage around webpack entry generation.
Written by Cursor Bugbot for commit 3b59b57. Configure here.
Summary by CodeRabbit