Reduce test coverage redundancies, audit docstrings#59
Reduce test coverage redundancies, audit docstrings#59alex-rawlings-yyc wants to merge 4 commits into
Conversation
alex-rawlings-yyc
added
the
up next
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ 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: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (23)
📝 WalkthroughWalkthroughRefactors tests to mock higher-level hooks and capture SegmentView props, tightens the ChangesTesting Infrastructure and Assertion Strategy
Parser Test Coverage Expansion
Type Safety and Component Props
Documentation and JSDoc Expansion
Possibly related PRs
Suggested labels
Suggested reviewers
🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 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)
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 |
imnasnainaec
left a comment
imnasnainaec
left a comment
There was a problem hiding this comment.
@imnasnainaec reviewed 15 files and all commit messages, and made 4 comments.
Reviewable status: 15 of 22 files reviewed, 1 unresolved discussion (waiting on alex-rawlings-yyc).
a discussion (no related file):
The mocking of PhraseBox and TokenChip in the *View.text.tsx files make me hesitant about #60. I like the idea of making sure unit tests only touch the file being tested. That keeps things consistent, modular, and organized. But it can be nice to test the interaction with a component and its child components for partial integration testing. Do we want to add extra test structure that results in more unit tests and less integration test coverage?
My concern is less relevant if we have some broader e2e and/or integration testing set up.
Wherever we do mock out components that are used by multiple other components, we may want the mocks to be defined just once for reuse.
src/__tests__/parsers/papi/usjBookExtractor.test.ts line 23 at r1 (raw file):
}); it('produces a stable contentHash independent of writingSystem', () => {
This test is fine, but I don't think we need to be generally zealous combining the yes and no cases of a situation into a single test.
src/__tests__/parsers/pt9/interlinearXmlParser.test.ts line 849 at r1 (raw file):
name: 'SyntaxError', message: expect.stringContaining( 'Invalid XML: Range missing or invalid Index/Length attributes (must be non-negative integers)',
⛏️ "Range missing" doesn't appear relevant to this test.
src/types/interlinearizer.d.ts line 796 at r1 (raw file):
* the phrase is a multi-word lexical entry — the Lexicon extension supports both kinds via * `IEntry.morphType = Phrase` (contiguous) or `DiscontiguousPhrase` (e.g. "ne … pas"). Exactly * one of `gloss` or `senseRef` may be set; setting both is a TypeScript type error. Omitting both
This won't be correct after #33
alex-rawlings-yyc
left a comment
alex-rawlings-yyc
left a comment
There was a problem hiding this comment.
@alex-rawlings-yyc made 2 comments.
Reviewable status: 15 of 22 files reviewed, 1 unresolved discussion (waiting on alex-rawlings-yyc and imnasnainaec).
a discussion (no related file):
Previously, imnasnainaec (D. Ror.) wrote…
The mocking of
PhraseBoxandTokenChipin the*View.text.tsxfiles make me hesitant about #60. I like the idea of making sure unit tests only touch the file being tested. That keeps things consistent, modular, and organized. But it can be nice to test the interaction with a component and its child components for partial integration testing. Do we want to add extra test structure that results in more unit tests and less integration test coverage?My concern is less relevant if we have some broader e2e and/or integration testing set up.
Wherever we do mock out components that are used by multiple other components, we may want the mocks to be defined just once for reuse.
I would prefer if we kept integration tests and unit tests pretty separate. However, I don't have a lot of experience in this regard, so I think we should get a good idea of what works best in practice. I just love modularity and dislike the conceptual uncleanliness of coverage overlap. However, coverage overlap may not be as messy or as redundant as I think it is. The output of a coverage tool doesn't capture a lot of the nuances of unit testing and I may be far too focused on having nice numbers and conceptual boundaries.
I think we should discuss this more on Thursday and try to come to a conclusion there.
src/types/interlinearizer.d.ts line 796 at r1 (raw file):
Previously, imnasnainaec (D. Ror.) wrote…
This won't be correct after #33
I'll make sure this is addressed there rather than preemptively updating it here.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/__tests__/hooks/useInterlinearizerBookData.test.ts (1)
1-1:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winFix Prettier formatting errors before merge.
The pipeline reports that
prettier --checkfailed. Runprettier --write .to fix formatting issues across all files in this PR.🤖 Prompt for 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. In `@src/__tests__/hooks/useInterlinearizerBookData.test.ts` at line 1, Prettier formatting failed for this PR; run the formatter across the repo (e.g., execute `prettier --write .`) and commit the resulting changes so tests pass; specifically ensure the test file useInterlinearizerBookData.test.ts and any other modified files are reformatted and re-committed to resolve the `prettier --check` failure.
🧹 Nitpick comments (1)
__mocks__/papi-frontend-react.ts (1)
22-22: 💤 Low valueConsider typing
propasstring | symbolfor Proxy correctness.The Proxy
gettrap's property parameter can be either astringor asymbol. While string-only typing works for current usage, usingprop: string | symbolwould be more precise.♻️ More precise typing
- get(_target, prop: string) { + get(_target, prop: string | symbol) { + if (typeof prop !== 'string') { + throw new Error(`useProjectData mock: symbol properties not supported.`); + } if (KNOWN_PROJECT_DATA_METHODS.has(prop)) {🤖 Prompt for 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. In `@__mocks__/papi-frontend-react.ts` at line 22, The Proxy get trap in __mocks__/papi-frontend-react.ts currently types the property parameter as prop: string but must accept symbols too; update the get(_target, prop: string) signature to prop: string | symbol to match the ProxyHandler.get definition and ensure correct typing across symbol property access in the mock. Reference the get trap function in this mock and adjust all related usage or type assertions to accommodate prop being string | symbol.
🤖 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.
Outside diff comments:
In `@src/__tests__/hooks/useInterlinearizerBookData.test.ts`:
- Line 1: Prettier formatting failed for this PR; run the formatter across the
repo (e.g., execute `prettier --write .`) and commit the resulting changes so
tests pass; specifically ensure the test file useInterlinearizerBookData.test.ts
and any other modified files are reformatted and re-committed to resolve the
`prettier --check` failure.
---
Nitpick comments:
In `@__mocks__/papi-frontend-react.ts`:
- Line 22: The Proxy get trap in __mocks__/papi-frontend-react.ts currently
types the property parameter as prop: string but must accept symbols too; update
the get(_target, prop: string) signature to prop: string | symbol to match the
ProxyHandler.get definition and ensure correct typing across symbol property
access in the mock. Reference the get trap function in this mock and adjust all
related usage or type assertions to accommodate prop being string | symbol.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 96f39dd1-7677-4e35-86ff-bf285db6b441
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (23)
__mocks__/papi-frontend-react.tssrc/__tests__/components/ContinuousView.test.tsxsrc/__tests__/components/Interlinearizer.test.tsxsrc/__tests__/components/InterlinearizerLoader.test.tsxsrc/__tests__/components/SegmentView.test.tsxsrc/__tests__/components/TokenChip.test.tsxsrc/__tests__/hooks/useInterlinearizerBookData.test.tssrc/__tests__/hooks/useOptimisticBooleanSetting.test.tssrc/__tests__/main.test.tssrc/__tests__/parsers/papi/bookTokenizer.test.tssrc/__tests__/parsers/papi/usjBookExtractor.test.tssrc/__tests__/parsers/pt9/interlinearXmlParser.test.tssrc/components/ContinuousView.tsxsrc/components/Interlinearizer.tsxsrc/components/InterlinearizerLoader.tsxsrc/components/ScriptureNavControls.tsxsrc/components/SegmentView.tsxsrc/hooks/useInterlinearizerBookData.tssrc/main.tssrc/parsers/papi/bookTokenizer.tssrc/parsers/papi/usjBookExtractor.tssrc/parsers/pt9/interlinearXmlParser.tssrc/types/interlinearizer.d.ts
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
imnasnainaec
left a comment
imnasnainaec
left a comment
There was a problem hiding this comment.
@imnasnainaec reviewed 8 files and all commit messages, and resolved 1 discussion.
Reviewable status: 19 of 24 files reviewed, all discussions resolved (waiting on alex-rawlings-yyc).
alex-rawlings-yyc
force-pushed
the
reduce-coverage-redundancies
branch
from
2f2b34e to
7e997a5
Compare
2 participants
This change is
Summary by CodeRabbit
Tests
Bug Fixes
Documentation