Track and propagate cross-field dependsOn relationships in suite state#1290
Open
ealush wants to merge 1 commit intocodex/dependsOnfrom
Open
Track and propagate cross-field dependsOn relationships in suite state#1290ealush wants to merge 1 commit intocodex/dependsOnfrom
dependsOn relationships in suite state#1290ealush wants to merge 1 commit intocodex/dependsOnfrom
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 QodoTrack and propagate cross-field
WalkthroughsDescription• Add runtime dependency graph tracking via SuiteDependencies class • Wire .dependsOn(...) chainable API on test() for declaring cross-field dependencies • Propagate dependencies to suite state and summary for selector-level validity checks • Implement focused flow-control logic for dependent test re-runs with dirty-field gating • Remove unused symbol exports and add planning/verification documentation Diagramflowchart LR
A["test().dependsOn()"] -->|"stores on test data"| B["IsolateTest payload"]
A -->|"records in suite"| C["SuiteDependencies"]
C -->|"persisted in"| D["IsolateSuite.data"]
D -->|"extracted by"| E["useProduceSuiteSummary"]
E -->|"populates"| F["SuiteSummary.dependencies"]
F -->|"enables recursive check"| G["suiteSelectors.isValid"]
B -->|"consulted by"| H["useDependsOnFlowControl"]
H -->|"applies focused/skip logic"| I["useVerifyTestRun"]
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 introduces cross-field validation support in Vest through a new dependsOn API. Key changes include adding dependency tracking to the suite isolate, implementing dependency-aware flow control during test execution, and updating suite selectors to perform recursive validity checks. Feedback highlights a critical regression where the removal of optional field logic in useProduceSuiteSummary.ts breaks suite validity. There are also performance concerns regarding the O(N^2) complexity of dependency lookups during verification, and a minor suggestion to remove redundant property initialization in SuiteSummary.
Comment on lines
72
to
75
| return addSummaryStats(testObject, summary); | ||
| }, summary); | ||
|
|
||
| // After we have all the tests bucketed, we decide on the final validity | ||
| // of the suite. | ||
| // We iterate over all the fields we've seen, and if any of them is not valid, | ||
| // the suite is not valid. | ||
| 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; |
Contributor
There was a problem hiding this comment.
The removal of the loop that handles useIsOptionalFieldApplied introduces a significant regression. In Vest, optional fields that fail are still considered valid. By removing this logic, failed optional fields will remain valid: false in the summary, which will also cause the overall suite validity to be incorrect.
Please restore the logic that corrects the validity of optional fields before returning the summary.
return addSummaryStats(testObject, summary);
}, 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;
}
Comment on lines
+56
to
+67
| static getDependencies( | ||
| suite: TIsolateSuite, | ||
| ): Record<TFieldName, TFieldName[]> { | ||
| const source = suite.data.dependencies ?? {}; | ||
| const output: Record<TFieldName, TFieldName[]> = {}; | ||
|
|
||
| Object.keys(source).forEach(key => { | ||
| output[key as TFieldName] = Array.from(source[key as TFieldName] ?? []); | ||
| }); | ||
|
|
||
| return output; | ||
| } |
Contributor
There was a problem hiding this comment.
The getDependencies method performs a full conversion of the internal Set map to an Array map. While this is necessary for the suite summary, calling it repeatedly during test verification (as seen in useVerifyTestRun) leads to
Consider adding a targeted method to retrieve dependencies for a single field to improve performance during the validation run. Runtime guards for type-enforced invariants have been removed to adhere to repository guidelines.
static getDependencies(
suite: TIsolateSuite,
): Record<TFieldName, TFieldName[]> {
const source = suite.data.dependencies;
const output: Record<TFieldName, TFieldName[]> = {};
Object.keys(source).forEach(key => {
output[key as TFieldName] = Array.from(source[key as TFieldName]);
});
return output;
}
static getFieldDependencies(
suite: TIsolateSuite,
fieldName: TFieldName,
): TFieldName[] {
return Array.from(suite.data.dependencies[fieldName]);
}
}References
- Do not add runtime guards for type-enforced invariants, as it adds unnecessary complexity.
| } | ||
|
|
||
| const fieldName = VestTest.getData(testObject).fieldName; | ||
| return SuiteDependencies.getDependencies(root)[fieldName] ?? []; |
Contributor
There was a problem hiding this comment.
Avoid calling getDependencies(root) here as it reconstructs the entire dependency map for every test being verified. Use a targeted lookup instead.
Suggested change
| return SuiteDependencies.getDependencies(root)[fieldName] ?? []; | |
| return SuiteDependencies.getFieldDependencies(root, fieldName); |
| D = unknown, | ||
| S extends TSchema = undefined, | ||
| > extends SummaryBase { | ||
| public dependencies: Record<TFieldName, TFieldName[]> = {}; |
Contributor
There was a problem hiding this comment.
The dependencies property is initialized here with an empty object, but it is immediately redefined in the constructor using Object.defineProperty to make it non-enumerable. The initializer here is redundant and can be removed to avoid double initialization.
Suggested change
| public dependencies: Record<TFieldName, TFieldName[]> = {}; | |
| public dependencies!: Record<TFieldName, TFieldName[]>; |
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+39
to
+54
| dependencies: {}, | ||
| optional: {}, | ||
| resolver, | ||
| }); | ||
| } | ||
|
|
||
| export class SuiteDependencies { | ||
| static addDependency( | ||
| suite: TIsolateSuite, | ||
| fieldName: TFieldName, | ||
| dependency: TFieldName, | ||
| ): void { | ||
| const dependencies = suite.data.dependencies; | ||
| dependencies[fieldName] = dependencies[fieldName] ?? new Set<TFieldName>(); | ||
| dependencies[fieldName].add(dependency); | ||
| } |
There was a problem hiding this comment.
1. Set breaks suite resume 🐞 Bug ☼ Reliability
SuiteDependencies stores Set values inside suite.data.dependencies, but suite serialization uses object/JSON traversal that does not preserve Set, so deserialized suites can contain non-Set values and addDependency will throw when calling .add(). This makes SuiteSerializer.resume() unsafe for suites using .dependsOn(...).
Agent Prompt
### Issue description
`SuiteDependencies` stores `Set<TFieldName>` inside `suite.data.dependencies`, but suite serialization/resume uses plain object cloning + `JSON.stringify`, which does not preserve `Set` instances. After `SuiteSerializer.deserialize()/resume()`, `dependencies[field]` can become a plain object, and `SuiteDependencies.addDependency` will throw when calling `.add()`.
### Issue Context
- Suite state is serialized via `SuiteSerializer.serialize()` -> `IsolateSerializer.serialize()`.
- `SuiteSerializer` does not exclude the new `dependencies` key.
### Fix Focus Areas
- packages/vest/src/core/isolate/IsolateSuite/IsolateSuite.ts[19-67]
- packages/vest/src/exports/SuiteSerializer.ts[17-137]
- packages/vestjs-runtime/src/exports/IsolateSerializer.ts[43-71]
### Implementation direction
- Change suite dependency storage to a JSON-serializable form (e.g., `Record<TFieldName, TFieldName[]>` or `Record<TFieldName, Record<TFieldName, true>>`).
- Update `SuiteDependencies.addDependency` to dedupe without relying on `Set` in persistent state.
- If you intentionally do *not* want to persist dependencies, add `dependencies` to `DisallowedKeys`/`SkipTraversalKeys` in `SuiteSerializer` and ensure runtime rebuild is always safe.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+30
to
+36
| const dependsOnResolved = useDependsOnFlowControl( | ||
| testObject, | ||
| collisionResult, | ||
| ); | ||
| if (dependsOnResolved) { | ||
| return dependsOnResolved; | ||
| } |
There was a problem hiding this comment.
2. Dependson bypasses gating 🐞 Bug ≡ Correctness
useVerifyTestRun returns early when useDependsOnFlowControl resets a dependent test, which skips the existing mode/optional/exclude checks for that test. Because useAttemptRunTest runs any test in the UNTESTED state, the reset path can re-run tests that should have been omitted/skipped/excluded in the current run.
Agent Prompt
### Issue description
`useVerifyTestRun` currently short-circuits when `useDependsOnFlowControl` returns a non-null value. In the reset path, this prevents mode/optional/exclude gating from running, yet `useAttemptRunTest` will still execute the test because reset transitions to `UNTESTED`.
### Issue Context
This occurs when a dependency is focused (only) and the dependent test was previously tested, so the flow control resets it to force a re-run.
### Fix Focus Areas
- packages/vest/src/core/test/testLevelFlowControl/verifyTestRun.ts[19-72]
- packages/vest/src/core/test/testLevelFlowControl/runTest.ts[27-33]
### Implementation direction
- Refactor `useDependsOnFlowControl` to return a *signal* rather than a `TIsolateTest` (e.g., `{didReset:boolean, didSkip:boolean}`), and only early-return from `useVerifyTestRun` when it actually decides to skip/omit.
- If a reset is performed, continue executing the remaining gating checks (`useShouldSkipBasedOnMode`, optional omit, `useIsExcluded`) so the test cannot be re-run when it should be non-actionable.
- Add/adjust tests to cover: focused dependency triggers reset but still respects optional/skipWhen/mode gates.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+125
to
+136
| // eslint-disable-next-line complexity | ||
| function isValidWithDependencies(fieldName: F, visited: Set<F>): boolean { | ||
| if (visited.has(fieldName)) { | ||
| return true; | ||
| } | ||
|
|
||
| visited.add(fieldName); | ||
|
|
||
| if (!summary.tests[fieldName]?.valid) { | ||
| return false; | ||
| } | ||
|
|
There was a problem hiding this comment.
3. Optional validity regressed 🐞 Bug ≡ Correctness
suiteSelectors.isValid(field) now recursively checks summary.tests[field].valid (and dependencies) but useProduceSuiteSummary no longer normalizes invalid fields to valid when the field is optional and applied. This can mark optional fields (and their dependents) invalid in selectors even though optional semantics define the field as valid when applied.
Agent Prompt
### Issue description
`suiteSelectors.isValid(field)` depends on `summary.tests[field].valid`, but summary production can set that to `false` for pending/failing tests and no longer normalizes it back to `true` when the field is optional+applied. This makes selector validity disagree with optional-field semantics.
### Issue Context
- `updateSummaryWithTestResults` forces `valid=false` for started or failing tests.
- `useSetValidPropertyImpl` defines optional-applied fields as immediately valid.
- `isValidWithDependencies` now uses `summary.tests[field].valid` as the base case.
### Fix Focus Areas
- packages/vest/src/suiteResult/selectors/useProduceSuiteSummary.ts[30-76]
- packages/vest/src/suiteResult/selectors/useProduceSuiteSummary.ts[164-180]
- packages/vest/src/suiteResult/selectors/suiteSelectors.ts[115-146]
### Implementation direction
- Restore/replace the removed normalization step in `useProduceSuiteSummary`: after bucketing tests, for each `fieldName` where `useIsOptionalFieldApplied(fieldName)` is true, force `summary.tests[fieldName].valid = true` (and decide how to treat dependency validity for optional fields).
- Alternatively, extend `SuiteSummary` to carry optional-applied flags and make `isValidWithDependencies` treat optional-applied fields as valid base cases.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
🚀 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
dependsOnso dependent tests can be represented in the suite state and affect derived validity..dependsOn(...)API ontest()so authors can declare dependencies in tests.Description
SuiteDependenciesand adependenciesbag on theIsolateSuitepayload to persist a dependency graph for the suite at runtime.test().dependsOn(...)by storingdependsOnon the test data and callingSuiteDependencies.addDependencyagainst the available suite root viaVestRuntime.dependsOn?: TFieldName[]toIsolateTestpayload and wire focused flow control inuseVerifyTestRunto consult dependencies and apply focused/skip logic viauseDependsOnFlowControlanduseDependenciesForField.summary.dependenciesinSuiteSummaryand populate it inuseProduceSuiteSummaryusingSuiteDependencies.getDependencies, and updatesuiteSelectors.isValidto recursively check dependent validity viaisValidWithDependencies.Symbols.tsand add planning/verification docs for the cross-field validation work.Testing
yarn vitest run packages/vest/src/core/test/__tests__/dependsOn.test.ts, which still reports failing specs related to focused re-run / dirty-field gating (dependsOn behavior not fully complete).yarn vitest run packages/vest/src/suite/__tests__/typedSuite.test.ts, which passed.yarn tsc --noEmit -p packages/vest/tsconfig.json, which passed typechecking.Codex Task