chore: remove leftover dependsOn artifacts#1289
Open
ealush wants to merge 2 commits into
Open
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
Review Summary by QodoRemove leftover dependsOn artifacts and unused symbols
WalkthroughsDescription• Remove unused DEPENDENCIES_SYMBOL and DEPENDS_ON_SYMBOL exports • Eliminate redundant suite validity loop in summary selector • Consolidate validity determination into useSetValidProperty call • Reduce code duplication and improve single-responsibility principle Diagramflowchart LR
A["Symbols.ts<br/>unused exports"] -->|delete| B["Cleanup"]
C["useProduceSuiteSummary.ts<br/>redundant validity loop"] -->|remove| B
B -->|consolidate| D["useSetValidProperty<br/>single validity source"]
File Changes1. packages/vest/src/core/Symbols.ts
|
Code Review by Qodo
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request removes symbol exports and logic for reconciling optional field validity within the suite summary. The review feedback highlights that these changes introduce regressions: specifically, removing the validity reconciliation loop causes optional fields with failing tests to be incorrectly marked as invalid and prevents the suite-level validity from correctly accounting for optional field status. Both the import and the logic loop should be restored to maintain correct reporting behavior.
| useSetValidProperty, | ||
| } from './useSetValidProperty'; | ||
| import { useIsOptionalFieldApplied } from '../../hooks/optional/optional'; | ||
|
|
Contributor
There was a problem hiding this comment.
This import is required for the field-level validity reconciliation logic. Removing it alongside the loop below causes a regression in how optional fields are reported in the suite summary.
import { useIsOptionalFieldApplied } from '../../hooks/optional/optional';| } | ||
| } | ||
|
|
||
| return summary; |
Contributor
There was a problem hiding this comment.
Removing this loop introduces a regression in the suite summary's validity reporting:
- Field-level validity: The summary.tests[fieldName].valid property will now incorrectly remain false for optional fields that have failing tests. This loop was responsible for correcting those to true based on the field's optional status.
- Suite-level validity: The PR relies on useSetValidProperty() (line 42) for the overall suite validity. However, useSetValidProperty() (when called without arguments) does not account for field optionality; it returns false if any test in the registry has failed, even if that test belongs to an applied optional field.
Restoring this logic ensures that both individual fields and the suite as a whole correctly reflect validity when optional fields are involved.
Suggested change
| return summary; | |
| for (const fieldName in summary.tests) { | |
| if (summary.tests[fieldName].valid === false) { | |
| if (useIsOptionalFieldApplied(fieldName as F).unwrap()) { | |
| summary.tests[fieldName].valid = true; | |
| } else { | |
| summary.valid = false; | |
| break; | |
| } | |
| } | |
| } | |
| return summary; |
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| return addSummaryStats(testObject, summary); | ||
| }, summary); | ||
|
|
||
| // After we have all the tests bucketed, we decide on the final validity |
There was a problem hiding this comment.
1. Optional validity drift 🐞 Bug ≡ Correctness
useProduceSuiteSummary no longer forces SuiteSummary.tests[field].valid to true when that field is an applied optional, so suiteResult.isValid(field) can return false even though useSetValidPropertyImpl(field) would return true. This can occur because the summary marks a field invalid on STARTED/FAILED tests while the optional-omission pass can set applied: true without touching STARTED tests.
Agent Prompt
### Issue description
`SuiteSummary.tests[field].valid` can remain `false` for a field that is an *applied optional*, causing `suiteResult.isValid(field)` to disagree with `useSetValidPropertyImpl(field)` (which returns `true` immediately when the optional condition is applied).
### Issue Context
- `suiteSelectors.isValid(field)` returns `summary.tests[field].valid`.
- The summary marks a field invalid when any test for it is `STARTED` or `FAILED`.
- Optional omission can set `applied: true` while skipping `STARTED` tests, so the field can become “optional-applied” while still having a STARTED test that keeps the summary’s `valid` flag `false`.
### Fix Focus Areas
Implement one of the following so that field validity in the produced summary honors applied optional state:
- Reintroduce a targeted post-pass that only adjusts `summary.tests[field].valid` for fields with `valid === false` when `useIsOptionalFieldApplied(field)` is true.
- Or integrate optional-applied handling into the per-test summary update path (so `nextSummaryKey.valid` cannot remain `false` when the field is optional-applied).
- packages/vest/src/suiteResult/selectors/useProduceSuiteSummary.ts[48-72]
- packages/vest/src/suiteResult/selectors/useProduceSuiteSummary.ts[139-182]
- packages/vest/src/suiteResult/selectors/suiteSelectors.ts[115-123]
- packages/vest/src/suiteResult/selectors/useSetValidProperty.ts[117-139]
- packages/vest/src/hooks/optional/omitOptionalFields.ts[33-60]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
🚀 Benchmark Results
Raw OutputPREVIOUS_RESULTS🚀 Benchmark Results
Raw Output |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
dependsOnimplementation to keep the suite summary logic single-responsibility and avoid unused code.useSetValidPropertyrather than performing an extra field iteration in the summary selector.Description
packages/vest/src/core/Symbols.tsfile that only exportedDEPENDENCIES_SYMBOLandDEPENDS_ON_SYMBOL.packages/vest/src/suiteResult/selectors/useProduceSuiteSummary.tsby removing the extra optional-field override loop and the now-unused import ofuseIsOptionalFieldApplied, relying on the existinguseSetValidPropertycall to compute suite validity.Testing
yarn vitest run packages/vest/src/suiteResult/__tests__/useProduceSuiteSummary.test.ts, which passed successfully.yarn vitest run packages/vest/src/core/test/__tests__/dependsOn.test.ts packages/vest/src/suite/__tests__/typedSuite.test.ts, wheretypedSuite.test.tspassed butdependsOn.test.tscontains existing failures (7 failing tests) unrelated to this cleanup.Codex Task