Add success severity with hooks, selectors and summary support#1262
Open
Add success severity with hooks, selectors and summary support#1262
success severity with hooks, selectors and summary support#1262Conversation
📝 WalkthroughWalkthroughAdds a new "success" severity across Vest: enums and summary fields, hooks (sync and hook-based), suite selectors and serialization support, core test severity APIs, test utilities, updated tests/snapshots, and documentation; no changes to existing error-handling semantics beyond severity classification. Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Test Body
participant Hook as useSuccess()/success()
participant VestTest as VestTest API
participant Producer as Summary Producer
participant Selector as Suite Selector
Test->>Hook: call success() or useSuccess().success()
Hook->>VestTest: VestTest.setSeverity(test, TestSeverity.Success)
VestTest->>VestTest: update test.data.severity to Success
Producer->>VestTest: iterate test objects
VestTest->>Producer: getSummarySeverity(test) -> SUCCESSES (if passing)
Producer->>Producer: increment successCount, push to successes[]
Selector->>Producer: hasSuccesses()/getSuccesses()
Producer->>Selector: return successes data
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly Related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 QodoAdd success and info severities with hooks, selectors and summary support
WalkthroughsDescription• Add success and info severity levels with dedicated hooks • Implement severity selectors and parser support for new levels • Update test aggregation to collect success/info only for passing tests • Extend suite summary with success/info counts and message arrays Diagramflowchart LR
A["TestSeverity enum"] -->|extends| B["VestTest severity methods"]
B -->|isSuccess, isInfo| C["Severity hooks"]
C -->|success, info, useSeverity| D["Public API exports"]
B -->|setSeverity| E["Suite summary aggregation"]
E -->|getSummarySeverity| F["SuiteSummary counts and arrays"]
F -->|successCount, infoCount| G["Suite selectors"]
G -->|hasSuccesses, getSuccesses, hasInfo, getInfo| H["Parser support"]
File Changes1. packages/vest/src/suiteResult/Severity.ts
|
Code Review by Qodo
1.
|
Contributor
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly expands the Vest validation framework by introducing Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![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 success and info severities, which is a great extension to the library's validation model. The changes are comprehensive, covering new hooks, selectors, and summary logic, and are well-tested. The implementation is consistent with the existing codebase patterns. I've found one minor opportunity for simplification in the summary production logic to improve code clarity.
Comment on lines
+234
to
+241
| function isErrorFailure(testObject: TIsolateTest): boolean { | ||
| return ( | ||
| VestTest.isFailing(testObject).unwrap() && | ||
| !VestTest.warns(testObject).unwrap() && | ||
| !VestTest.isSuccess(testObject).unwrap() && | ||
| !VestTest.isInfo(testObject).unwrap() | ||
| ); | ||
| } |
Contributor
There was a problem hiding this comment.
The checks for !warns(), !isSuccess(), and !isInfo() are redundant here. The VestTest.isFailing() check is sufficient on its own.
A test's status is only set to TestStatus.FAILED if its severity was TestSeverity.Error and it failed. If the severity is Error, it cannot simultaneously be Warning, Success, or Info.
Simplifying this function will improve readability and reduce complexity.
function isErrorFailure(testObject: TIsolateTest): boolean {
return VestTest.isFailing(testObject).unwrap();
}
🚀 Benchmark Results
Raw OutputPREVIOUS_RESULTS🚀 Benchmark Results
Raw Output🚀 Benchmark Results
Raw Output🚀 Benchmark Results
Raw Output |
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
packages/vest/src/core/test/helpers/__tests__/nonMatchingSeverityProfile.test.ts (1)
54-65: Add an explicitINFOseverity assertion in this matrix.This block now validates
SUCCESS, but notINFOin the same non-matching matrix. A small companion case here would make regressions easier to catch.✅ Suggested test addition
describe('When test is success', () => { it('should return true for errors and false for successes', () => { VestTest.setSeverity(testObject, TestSeverity.Success); expect( nonMatchingSeverityProfile(Severity.ERRORS, testObject).unwrap(), ).toBe(true); expect( nonMatchingSeverityProfile(Severity.SUCCESSES, testObject).unwrap(), ).toBe(false); }); }); + + describe('When test is info', () => { + it('should return true for errors and false for info', () => { + VestTest.setSeverity(testObject, TestSeverity.Info); + + expect( + nonMatchingSeverityProfile(Severity.ERRORS, testObject).unwrap(), + ).toBe(true); + expect( + nonMatchingSeverityProfile(Severity.INFO, testObject).unwrap(), + ).toBe(false); + }); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/vest/src/core/test/helpers/__tests__/nonMatchingSeverityProfile.test.ts` around lines 54 - 65, Add an explicit assertion for INFO in the "When test is success" matrix: after calling VestTest.setSeverity(testObject, TestSeverity.Success) and the existing nonMatchingSeverityProfile checks, also call nonMatchingSeverityProfile(Severity.INFO, testObject).unwrap() and assert the expected boolean (likely false for a success), so the test covers INFO alongside ERRORS and SUCCESSES; target the nonMatchingSeverityProfile function and the VestTest.setSeverity / TestSeverity.Success setup when adding this assertion.packages/vest/src/hooks/__tests__/severityHooks.test.ts (1)
6-26: Assert severity outcomes explicitly, not onlyisValid().On Line 25,
isValid()alone won’t catch regressions insuccess()/info()routing or the “last severity wins” behavior.Suggested test tightening
import { describe, expect, it } from 'vitest'; import { create, enforce, info, success, test, warn } from '../../vest'; +import { parse } from '../../exports/parser'; @@ const res = suite.run(); + const parsed = parse(res); expect(res.isValid()).toBe(true); + expect(parsed.success('field1')).toBe(true); + expect(parsed.info('field2')).toBe(true); + expect(parsed.success('field3')).toBe(true); + expect(parsed.warning('field3')).toBe(false); }); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/vest/src/hooks/__tests__/severityHooks.test.ts` around lines 6 - 26, The test currently only checks res.isValid() which won't verify that the severity hooks (success(), info(), warn()) and "last severity wins" behavior are applied to individual tests; update the test to assert each test's severity explicitly by inspecting the suite run result (e.g., the individual test results returned by suite.run() or result.getTests()/res.tests) and asserting that the 'field1' result has severity 'success' (or equivalent success marker), 'field2' has 'info', and 'field3' reflects the last call ('success' after warn). Locate and update assertions around suite.run() and res to validate the per-test severity values rather than only isValid().packages/vest/src/exports/__tests__/parser.test.ts (1)
469-507: Add field-level assertions forparse().successandparse().info.These tests currently verify suite-level behavior only; the new methods also accept
fieldNameand should be covered.Suggested additions
describe('parse().success', () => { @@ it('Should return false when the suite has no successes', () => { expect(parse(suiteDummy.failing()).success()).toBe(false); }); + it('Should return true when provided field has success', () => { + expect(parse(suiteDummy.success('username')).success('username')).toBe(true); + }); @@ describe('parse().info', () => { @@ it('Should return false when the suite has no info', () => { expect(parse(suiteDummy.failing()).info()).toBe(false); }); + it('Should return true when provided field has info', () => { + expect(parse(suiteDummy.info('username')).info('username')).toBe(true); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/vest/src/exports/__tests__/parser.test.ts` around lines 469 - 507, Add tests that exercise the field-level overloads of parse().success(fieldName) and parse().info(fieldName): for each of success and info, add assertions that passing a matching field name returns true and a non-matching or missing field returns false, and duplicate these checks for both raw and serialized inputs (use parse(suiteDummy.success()).success('fieldName'), parse(ser(suiteDummy.success())).success('fieldName'), parse(suiteDummy.failing()).success('fieldName') etc., and the analogous parse(...).info('fieldName') cases). Ensure you cover true/false expectations for both suiteDummy.success()/info() and suiteDummy.failing(), and include tests for serialized results via ser(...).packages/vest/src/suiteResult/selectors/__tests__/successAndInfoSelectors.test.ts (1)
16-32: Pin the silent-drop contract for failed positive severities.This covers the failed
success()path, but it doesn't guard against that case being reclassified as a warning, and it doesn't exercise the separate failed-info()branch.🧪 Suggested coverage
test('failCase', 'should not be appended if enforcement fails', () => { success(); enforce(1).equals(2); }); + test('failInfo', 'should not be appended if enforcement fails', () => { + info(); + enforce(1).equals(2); + }); }); const res = suite.run(); @@ expect(res.getSuccesses('pass')).toEqual(['good password']); expect(res.getInfo('user')).toEqual(['auto formatted']); expect(res.hasSuccesses('failCase')).toBe(false); + expect(res.hasWarnings('failCase')).toBe(false); + expect(res.hasInfo('failInfo')).toBe(false); + expect(res.hasWarnings('failInfo')).toBe(false); expect(res.isValid()).toBe(true);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/vest/src/suiteResult/selectors/__tests__/successAndInfoSelectors.test.ts` around lines 16 - 32, Add tests to pin the "silent-drop" behavior for failed positive severities by covering two missing cases: (1) verify that a failed success() call cannot be reclassified as a warning by exercising enforce(...).equals(...) on a success-labeled test (use the same suite, call success(); enforce(...).equals(...); run suite.run() and assert hasSuccesses('...') is false and hasInfo/hasWarnings remains as expected), and (2) add a separate failing info() branch test to confirm failed info() is silently dropped (call info('msg'); enforce(...).equals(...); run and assert getInfo(...) does not include 'msg' and isValid() state). Target functions/selectors: success(), info(), enforce(), suite.run(), hasSuccesses(), hasInfo(), getSuccesses(), getInfo(), isValid() so the assertions match the intended silent-drop contract.plans/generalized_severities_summary.md (1)
7-11: Consider varying repeated bullet/step openers for readability.Several consecutive items start with the same verb (“Added” / “Updated”). Rewording a few improves scanability without changing meaning.
Also applies to: 20-22
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plans/generalized_severities_summary.md` around lines 7 - 11, The list repeats the same opener ("Added"/"Updated") and should vary phrasing for readability; edit the bullets that mention the SuiteResult selectors (`hasSuccesses`, `getSuccesses`, `hasInfo`, `getInfo`, `hasSuccessesByGroup`, `getSuccessesByGroup`, `hasInfoByGroup`, `getInfoByGroup`) to use different verbs or structures (e.g., "Introduced", "Added support for", "Added selectors:", "Also added") so consecutive items don't begin identically while retaining original meaning.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/vest/src/core/isolate/IsolateTest/VestTest.ts`:
- Around line 207-213: The bug is that VestTest.reset() doesn't restore test
severity, so VestTest.fail() can read a stale severity (TestSeverity.Warning)
and downgrade fails; update VestTest.reset() to explicitly set the test's
severity back to TestSeverity.Error via VestTest.getData(test).severity =
TestSeverity.Error (or using the existing setter if one exists) for the same
test objects that VestTest.warn() modifies; ensure this reset happens before
reruns so VestTest.fail() and VestTest.setStatus(...) behave correctly.
In `@packages/vest/src/hooks/__tests__/useSeverity.test.ts`:
- Around line 11-23: The test claims to exercise warn, success and info but
never calls info(); update the test inside the create(...) callback where you
obtain severity via useSeverity() (the block that currently calls
severity.warn() and severity.success()) to also call severity.info() so all
three setters are invoked; keep the rest of the assertion using
VestTest.cast(t).unwrap() and VestTest.isSuccess(...) unchanged.
In `@website/static/llms-full.txt`:
- Around line 5562-5565: The TTL option description contains a typo ("test
blocked will be re-run"); update the table cell for the `ttl` option (adjacent
to `cacheSize`) to replace that phrase with the correct wording such as
"memoized block will be re-run" or "test block will be re-run" so the
description reads: "Time-to-Live in milliseconds. If the cached result is older
than the `ttl`, the cache will be invalidated and the memoized block will be
re-run." Ensure you edit the `ttl` description text only.
---
Nitpick comments:
In
`@packages/vest/src/core/test/helpers/__tests__/nonMatchingSeverityProfile.test.ts`:
- Around line 54-65: Add an explicit assertion for INFO in the "When test is
success" matrix: after calling VestTest.setSeverity(testObject,
TestSeverity.Success) and the existing nonMatchingSeverityProfile checks, also
call nonMatchingSeverityProfile(Severity.INFO, testObject).unwrap() and assert
the expected boolean (likely false for a success), so the test covers INFO
alongside ERRORS and SUCCESSES; target the nonMatchingSeverityProfile function
and the VestTest.setSeverity / TestSeverity.Success setup when adding this
assertion.
In `@packages/vest/src/exports/__tests__/parser.test.ts`:
- Around line 469-507: Add tests that exercise the field-level overloads of
parse().success(fieldName) and parse().info(fieldName): for each of success and
info, add assertions that passing a matching field name returns true and a
non-matching or missing field returns false, and duplicate these checks for both
raw and serialized inputs (use parse(suiteDummy.success()).success('fieldName'),
parse(ser(suiteDummy.success())).success('fieldName'),
parse(suiteDummy.failing()).success('fieldName') etc., and the analogous
parse(...).info('fieldName') cases). Ensure you cover true/false expectations
for both suiteDummy.success()/info() and suiteDummy.failing(), and include tests
for serialized results via ser(...).
In `@packages/vest/src/hooks/__tests__/severityHooks.test.ts`:
- Around line 6-26: The test currently only checks res.isValid() which won't
verify that the severity hooks (success(), info(), warn()) and "last severity
wins" behavior are applied to individual tests; update the test to assert each
test's severity explicitly by inspecting the suite run result (e.g., the
individual test results returned by suite.run() or result.getTests()/res.tests)
and asserting that the 'field1' result has severity 'success' (or equivalent
success marker), 'field2' has 'info', and 'field3' reflects the last call
('success' after warn). Locate and update assertions around suite.run() and res
to validate the per-test severity values rather than only isValid().
In
`@packages/vest/src/suiteResult/selectors/__tests__/successAndInfoSelectors.test.ts`:
- Around line 16-32: Add tests to pin the "silent-drop" behavior for failed
positive severities by covering two missing cases: (1) verify that a failed
success() call cannot be reclassified as a warning by exercising
enforce(...).equals(...) on a success-labeled test (use the same suite, call
success(); enforce(...).equals(...); run suite.run() and assert
hasSuccesses('...') is false and hasInfo/hasWarnings remains as expected), and
(2) add a separate failing info() branch test to confirm failed info() is
silently dropped (call info('msg'); enforce(...).equals(...); run and assert
getInfo(...) does not include 'msg' and isValid() state). Target
functions/selectors: success(), info(), enforce(), suite.run(), hasSuccesses(),
hasInfo(), getSuccesses(), getInfo(), isValid() so the assertions match the
intended silent-drop contract.
In `@plans/generalized_severities_summary.md`:
- Around line 7-11: The list repeats the same opener ("Added"/"Updated") and
should vary phrasing for readability; edit the bullets that mention the
SuiteResult selectors (`hasSuccesses`, `getSuccesses`, `hasInfo`, `getInfo`,
`hasSuccessesByGroup`, `getSuccessesByGroup`, `hasInfoByGroup`,
`getInfoByGroup`) to use different verbs or structures (e.g., "Introduced",
"Added support for", "Added selectors:", "Also added") so consecutive items
don't begin identically while retaining original meaning.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0b534b07-509f-40c7-a17f-1f82c4b8ed5c
⛔ Files ignored due to path filters (17)
packages/vest/src/__tests__/__snapshots__/integration.async-tests.test.ts.snapis excluded by!**/*.snappackages/vest/src/__tests__/__snapshots__/integration.base.test.ts.snapis excluded by!**/*.snappackages/vest/src/__tests__/__snapshots__/integration.stateful-async.test.ts.snapis excluded by!**/*.snappackages/vest/src/__tests__/__snapshots__/integration.stateful-tests.test.ts.snapis excluded by!**/*.snappackages/vest/src/core/__tests__/__snapshots__/runtime.test.ts.snapis excluded by!**/*.snappackages/vest/src/core/test/__tests__/__snapshots__/test.test.ts.snapis excluded by!**/*.snappackages/vest/src/exports/__tests__/__snapshots__/memo.test.ts.snapis excluded by!**/*.snappackages/vest/src/hooks/__tests__/__snapshots__/include.test.ts.snapis excluded by!**/*.snappackages/vest/src/hooks/focused/__tests__/__snapshots__/focused.test.ts.snapis excluded by!**/*.snappackages/vest/src/isolates/__tests__/__snapshots__/group.test.ts.snapis excluded by!**/*.snappackages/vest/src/isolates/__tests__/__snapshots__/omitWhen.test.ts.snapis excluded by!**/*.snappackages/vest/src/isolates/__tests__/__snapshots__/skipWhen.test.ts.snapis excluded by!**/*.snappackages/vest/src/suite/__tests__/__snapshots__/create.test.ts.snapis excluded by!**/*.snappackages/vest/src/suite/__tests__/__snapshots__/focus.test.ts.snapis excluded by!**/*.snappackages/vest/src/suite/__tests__/__snapshots__/suite.dump.test.ts.snapis excluded by!**/*.snappackages/vest/src/suite/after/__tests__/__snapshots__/afterEach.test.ts.snapis excluded by!**/*.snappackages/vest/src/suiteResult/__tests__/__snapshots__/useProduceSuiteSummary.test.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (29)
packages/vest/src/__tests__/integration.stateful-tests.test.tspackages/vest/src/core/isolate/IsolateTest/VestTest.tspackages/vest/src/core/isolate/IsolateTest/__tests__/severity.test.tspackages/vest/src/core/test/__tests__/key.test.tspackages/vest/src/core/test/__tests__/merging_of_previous_test_runs.test.tspackages/vest/src/core/test/helpers/__tests__/nonMatchingSeverityProfile.test.tspackages/vest/src/core/test/helpers/nonMatchingSeverityProfile.tspackages/vest/src/errors/ErrorStrings.tspackages/vest/src/exports/__tests__/classnames.test.tspackages/vest/src/exports/__tests__/parser.test.tspackages/vest/src/exports/parser.tspackages/vest/src/hooks/__tests__/severityHooks.test.tspackages/vest/src/hooks/__tests__/useSeverity.test.tspackages/vest/src/hooks/info.tspackages/vest/src/hooks/success.tspackages/vest/src/hooks/useInfo.tspackages/vest/src/hooks/useSeverity.tspackages/vest/src/hooks/useSuccess.tspackages/vest/src/suite/__tests__/resetField.test.tspackages/vest/src/suiteResult/Severity.tspackages/vest/src/suiteResult/SuiteResultTypes.tspackages/vest/src/suiteResult/selectors/__tests__/successAndInfoSelectors.test.tspackages/vest/src/suiteResult/selectors/suiteSelectors.tspackages/vest/src/suiteResult/selectors/useProduceSuiteSummary.tspackages/vest/src/testUtils/suiteDummy.tspackages/vest/src/testUtils/testDummy.tspackages/vest/src/vest.tsplans/generalized_severities_summary.mdwebsite/static/llms-full.txt
Comment on lines
+11
to
+23
| it('should expose warn, success and info severity setters', () => { | ||
| let t; | ||
|
|
||
| create(() => { | ||
| t = test(faker.lorem.word(), faker.lorem.sentence(), () => { | ||
| const severity = useSeverity(); | ||
| severity.warn(); | ||
| severity.success(); | ||
| }); | ||
| }).run(); | ||
|
|
||
| expect(VestTest.isSuccess(VestTest.cast(t).unwrap()).unwrap()).toBe(true); | ||
| }); |
There was a problem hiding this comment.
Test intent mentions info, but info() is never exercised.
Line 11 claims all three setters are covered, but Lines 17–19 only invoke warn and success.
Minimal fix to match the test contract
it('should expose warn, success and info severity setters', () => {
@@
t = test(faker.lorem.word(), faker.lorem.sentence(), () => {
const severity = useSeverity();
severity.warn();
severity.success();
+ severity.info();
});
}).run();
- expect(VestTest.isSuccess(VestTest.cast(t).unwrap()).unwrap()).toBe(true);
+ expect(VestTest.isInfo(VestTest.cast(t).unwrap()).unwrap()).toBe(true);
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| it('should expose warn, success and info severity setters', () => { | |
| let t; | |
| create(() => { | |
| t = test(faker.lorem.word(), faker.lorem.sentence(), () => { | |
| const severity = useSeverity(); | |
| severity.warn(); | |
| severity.success(); | |
| }); | |
| }).run(); | |
| expect(VestTest.isSuccess(VestTest.cast(t).unwrap()).unwrap()).toBe(true); | |
| }); | |
| it('should expose warn, success and info severity setters', () => { | |
| let t; | |
| create(() => { | |
| t = test(faker.lorem.word(), faker.lorem.sentence(), () => { | |
| const severity = useSeverity(); | |
| severity.warn(); | |
| severity.success(); | |
| severity.info(); | |
| }); | |
| }).run(); | |
| expect(VestTest.isInfo(VestTest.cast(t).unwrap()).unwrap()).toBe(true); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/vest/src/hooks/__tests__/useSeverity.test.ts` around lines 11 - 23,
The test claims to exercise warn, success and info but never calls info();
update the test inside the create(...) callback where you obtain severity via
useSeverity() (the block that currently calls severity.warn() and
severity.success()) to also call severity.info() so all three setters are
invoked; keep the rest of the assertion using VestTest.cast(t).unwrap() and
VestTest.isSuccess(...) unchanged.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/vest/src/hooks/__tests__/useSeverity.test.ts (1)
11-23:⚠️ Potential issue | 🟡 MinorTest description is inconsistent with the PR changes and actual test behavior.
The test description on Line 11 claims to test "error, warn, success and info severity setters", but:
- Per the PR objectives,
infowas removed from the severity model- The test body only calls
severity.warn()andseverity.success()(Lines 17-18), noterror()orinfo()The description should be updated to accurately reflect what's being tested.
📝 Suggested fix
- it('should expose error, warn, success and info severity setters', () => { + it('should expose warn and success severity setters', () => {Or, if the intent is to also verify
error():- it('should expose error, warn, success and info severity setters', () => { + it('should expose error, warn, and success severity setters', () => { let t; create(() => { t = test(faker.lorem.word(), faker.lorem.sentence(), () => { const severity = useSeverity(); + severity.error(); severity.warn(); severity.success(); }); }).run();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/vest/src/hooks/__tests__/useSeverity.test.ts` around lines 11 - 23, Update the test description string to match the actual assertions: change the it(...) title that currently says 'should expose error, warn, success and info severity setters' to something like 'should expose warn and success severity setters' since the test only calls useSeverity().warn() and useSeverity().success() (functions referenced as useSeverity, severity.warn, severity.success) and the 'info' severity was removed; alternatively, if you want to also verify error(), modify the test body to call severity.error() and assert accordingly, but do not leave the description and behavior mismatched.
🧹 Nitpick comments (3)
packages/vest/src/testUtils/testDummy.ts (1)
117-127: Consider adding an async variant for consistency.The test dummy exports async variants for other test types (
passingWarningAsync,failingAsync, etc.) but lacks apassingSuccessAsyncvariant. This may be intentional if async success tests aren't needed, but it creates an asymmetry in the API.♻️ Optional: Add async success helper
+const createPassingSuccessAsync = ( + name = faker.lorem.word(), + { message = faker.lorem.words(), time = 0 } = {}, +) => + vestTest( + name as TFieldName, + message, + vi.fn(async () => { + success(); + await wait(time); + }), + ); + const testDummy = () => ({ failing: createFailing, failingAsync: createFailingAsync, failingWarning: createFailingWarning, failingWarningAsync: createFailingWarningAsync, passing: createPassing, passingAsync: createPassingAsync, passingSuccess: createPassingSuccess, + passingSuccessAsync: createPassingSuccessAsync, passingWarning: createPassingWarning, passingWarningAsync: createPassingWarningAsync, });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/vest/src/testUtils/testDummy.ts` around lines 117 - 127, testDummy currently exposes a sync passingSuccess helper but no async counterpart, creating an inconsistent API; add a passingSuccessAsync entry to the returned object that maps to the async helper (e.g., createPassingSuccessAsync) and ensure that the async helper function (createPassingSuccessAsync) exists and behaves like passingSuccess but returns a resolved Promise so it matches other *Async helpers; update the testDummy return object to include passingSuccessAsync alongside passingSuccess.packages/vest/src/suiteResult/selectors/suiteSelectors.ts (1)
396-398: Avoid exposing failure-centric types through the success API.
getSuccesses*returningFailureMessagesmakes the public TypeScript surface read as though successes are failures. A neutral alias here (for exampleSeverityMessages) would keep the API self-descriptive as more non-error severities are added.Also applies to: 417-425
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/vest/src/suiteResult/selectors/suiteSelectors.ts` around lines 396 - 398, The public signatures for getSuccesses currently expose the failure-centric type FailureMessages; update the API to use a neutral alias (e.g., SeverityMessages) instead of FailureMessages so success-related APIs do not reference "failure" types. Replace FailureMessages with the new alias in all getSuccesses overloads (the group declared as getSuccesses(): FailureMessages; getSuccesses(fieldName: InputFieldName<F>): string[]; getSuccesses(fieldName?: InputFieldName<F>): string[] | FailureMessages;) and the analogous overloads at the later block (lines referenced in the review), and ensure the underlying type alias (SeverityMessages = FailureMessages) is defined and exported where appropriate so existing internals keep working while the public surface reads neutrally.packages/vest/src/suiteResult/selectors/__tests__/successAndInfoSelectors.test.ts (1)
132-179: ThesegetMessageprecedence cases are duplicated.This block overlaps with
packages/vest/src/suiteResult/selectors/__tests__/getFailure.test.tsat Lines 278-317. Keeping the priority matrix in one place would reduce drift and keep this file focused on success-selector behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/vest/src/suiteResult/selectors/__tests__/successAndInfoSelectors.test.ts` around lines 132 - 179, The four duplicated integration tests for getMessage (covering precedence of error > warn > success and undefined when no success) should be removed from this file and kept in a single canonical location to avoid drift; locate the block starting with "describe('getMessage integration')" that contains tests invoking create(), test(...), success(), warn(), and suite.run(), then either delete those it() cases here (or replace them with a minimal reference to the canonical test) so the canonical versions in getFailure.test.ts remain as the single source of truth; ensure any shared setup uses the same create/test/success/warn helpers if you extract to a shared helper.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/vest/src/errors/ErrorStrings.ts`:
- Around line 7-12: The error messages have inconsistent capitalization: update
the SUCCESS_MUST_BE_CALLED_FROM_TEST constant in ErrorStrings.ts so it matches
the style of WARN_MUST_BE_CALLED_FROM_TEST (capitalize the word "Success"
instead of "success"); locate the SUCCESS_MUST_BE_CALLED_FROM_TEST symbol and
change its string to "Success must be called from within the body of a test
function" (leave USE_SUCCESS_MUST_BE_CALLED_FROM_TEST and other messages
unchanged).
---
Duplicate comments:
In `@packages/vest/src/hooks/__tests__/useSeverity.test.ts`:
- Around line 11-23: Update the test description string to match the actual
assertions: change the it(...) title that currently says 'should expose error,
warn, success and info severity setters' to something like 'should expose warn
and success severity setters' since the test only calls useSeverity().warn() and
useSeverity().success() (functions referenced as useSeverity, severity.warn,
severity.success) and the 'info' severity was removed; alternatively, if you
want to also verify error(), modify the test body to call severity.error() and
assert accordingly, but do not leave the description and behavior mismatched.
---
Nitpick comments:
In
`@packages/vest/src/suiteResult/selectors/__tests__/successAndInfoSelectors.test.ts`:
- Around line 132-179: The four duplicated integration tests for getMessage
(covering precedence of error > warn > success and undefined when no success)
should be removed from this file and kept in a single canonical location to
avoid drift; locate the block starting with "describe('getMessage integration')"
that contains tests invoking create(), test(...), success(), warn(), and
suite.run(), then either delete those it() cases here (or replace them with a
minimal reference to the canonical test) so the canonical versions in
getFailure.test.ts remain as the single source of truth; ensure any shared setup
uses the same create/test/success/warn helpers if you extract to a shared
helper.
In `@packages/vest/src/suiteResult/selectors/suiteSelectors.ts`:
- Around line 396-398: The public signatures for getSuccesses currently expose
the failure-centric type FailureMessages; update the API to use a neutral alias
(e.g., SeverityMessages) instead of FailureMessages so success-related APIs do
not reference "failure" types. Replace FailureMessages with the new alias in all
getSuccesses overloads (the group declared as getSuccesses(): FailureMessages;
getSuccesses(fieldName: InputFieldName<F>): string[]; getSuccesses(fieldName?:
InputFieldName<F>): string[] | FailureMessages;) and the analogous overloads at
the later block (lines referenced in the review), and ensure the underlying type
alias (SeverityMessages = FailureMessages) is defined and exported where
appropriate so existing internals keep working while the public surface reads
neutrally.
In `@packages/vest/src/testUtils/testDummy.ts`:
- Around line 117-127: testDummy currently exposes a sync passingSuccess helper
but no async counterpart, creating an inconsistent API; add a
passingSuccessAsync entry to the returned object that maps to the async helper
(e.g., createPassingSuccessAsync) and ensure that the async helper function
(createPassingSuccessAsync) exists and behaves like passingSuccess but returns a
resolved Promise so it matches other *Async helpers; update the testDummy return
object to include passingSuccessAsync alongside passingSuccess.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4684e66e-a4f4-4066-956c-474e383f56ec
⛔ Files ignored due to path filters (17)
packages/vest/src/__tests__/__snapshots__/integration.async-tests.test.ts.snapis excluded by!**/*.snappackages/vest/src/__tests__/__snapshots__/integration.base.test.ts.snapis excluded by!**/*.snappackages/vest/src/__tests__/__snapshots__/integration.stateful-async.test.ts.snapis excluded by!**/*.snappackages/vest/src/__tests__/__snapshots__/integration.stateful-tests.test.ts.snapis excluded by!**/*.snappackages/vest/src/core/__tests__/__snapshots__/runtime.test.ts.snapis excluded by!**/*.snappackages/vest/src/core/test/__tests__/__snapshots__/test.test.ts.snapis excluded by!**/*.snappackages/vest/src/exports/__tests__/__snapshots__/memo.test.ts.snapis excluded by!**/*.snappackages/vest/src/hooks/__tests__/__snapshots__/include.test.ts.snapis excluded by!**/*.snappackages/vest/src/hooks/focused/__tests__/__snapshots__/focused.test.ts.snapis excluded by!**/*.snappackages/vest/src/isolates/__tests__/__snapshots__/group.test.ts.snapis excluded by!**/*.snappackages/vest/src/isolates/__tests__/__snapshots__/omitWhen.test.ts.snapis excluded by!**/*.snappackages/vest/src/isolates/__tests__/__snapshots__/skipWhen.test.ts.snapis excluded by!**/*.snappackages/vest/src/suite/__tests__/__snapshots__/create.test.ts.snapis excluded by!**/*.snappackages/vest/src/suite/__tests__/__snapshots__/focus.test.ts.snapis excluded by!**/*.snappackages/vest/src/suite/__tests__/__snapshots__/suite.dump.test.ts.snapis excluded by!**/*.snappackages/vest/src/suite/after/__tests__/__snapshots__/afterEach.test.ts.snapis excluded by!**/*.snappackages/vest/src/suiteResult/__tests__/__snapshots__/useProduceSuiteSummary.test.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (23)
packages/vest/src/__tests__/integration.stateful-tests.test.tspackages/vest/src/core/isolate/IsolateTest/VestTest.tspackages/vest/src/core/isolate/IsolateTest/__tests__/severity.test.tspackages/vest/src/core/test/__tests__/key.test.tspackages/vest/src/core/test/__tests__/merging_of_previous_test_runs.test.tspackages/vest/src/core/test/helpers/nonMatchingSeverityProfile.tspackages/vest/src/errors/ErrorStrings.tspackages/vest/src/exports/__tests__/classnames.test.tspackages/vest/src/exports/__tests__/parser.test.tspackages/vest/src/exports/parser.tspackages/vest/src/hooks/__tests__/severityHooks.test.tspackages/vest/src/hooks/__tests__/useSeverity.test.tspackages/vest/src/hooks/useSeverity.tspackages/vest/src/suite/__tests__/resetField.test.tspackages/vest/src/suiteResult/Severity.tspackages/vest/src/suiteResult/SuiteResultTypes.tspackages/vest/src/suiteResult/selectors/__tests__/getFailure.test.tspackages/vest/src/suiteResult/selectors/__tests__/successAndInfoSelectors.test.tspackages/vest/src/suiteResult/selectors/suiteSelectors.tspackages/vest/src/suiteResult/selectors/useProduceSuiteSummary.tspackages/vest/src/testUtils/suiteDummy.tspackages/vest/src/testUtils/testDummy.tspackages/vest/src/vest.ts
🚧 Files skipped from review as they are similar to previous changes (11)
- packages/vest/src/suite/tests/resetField.test.ts
- packages/vest/src/core/isolate/IsolateTest/VestTest.ts
- packages/vest/src/testUtils/suiteDummy.ts
- packages/vest/src/vest.ts
- packages/vest/src/exports/tests/classnames.test.ts
- packages/vest/src/core/test/helpers/nonMatchingSeverityProfile.ts
- packages/vest/src/hooks/tests/severityHooks.test.ts
- packages/vest/src/tests/integration.stateful-tests.test.ts
- packages/vest/src/exports/tests/parser.test.ts
- packages/vest/src/core/isolate/IsolateTest/tests/severity.test.ts
- packages/vest/src/exports/parser.ts
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
packages/vest/src/testUtils/testDummy.ts (1)
65-75: Inconsistent signature with other async helpers.Other async helpers in this file (
createPassingAsync,createFailingAsync,createPassingWarningAsync) accept{ message, time = 0 }as the second parameter and useawait wait(time)to simulate async delays. This helper takesmessagedirectly and lacks thetimeparameter.♻️ Proposed fix to match async helper pattern
const createPassingSuccessAsync = ( name = faker.lorem.word(), - message = faker.lorem.words(), + { message = faker.lorem.words(), time = 0 } = {}, ) => vestTest( name as TFieldName, message, vi.fn(async () => { success(); + await wait(time); }), );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/vest/src/testUtils/testDummy.ts` around lines 65 - 75, createPassingSuccessAsync currently takes (name, message) and doesn't match other async helpers' signature; change its second parameter to an options object ({ message, time = 0 }) and inside the vi.fn async callback call await wait(time) before invoking success() so it mirrors createPassingAsync/createFailingAsync/createPassingWarningAsync; update references to vestTest, TFieldName, vi.fn, success, and wait accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/vest/src/hooks/optional/mode.ts`:
- Around line 52-55: The early return in useShouldSkipBasedOnMode that
unconditionally returns false for VestTest.isSuccess(testObject) must be removed
or deferred so mode semantics are enforced first; update
useShouldSkipBasedOnMode to evaluate the current mode (ONE and EAGER) and prior
failure state before short-circuiting success tests — i.e., perform mode and
failure checks (handling ONE/EAGER) first, and only allow a success test to run
(VestTest.isSuccess) if the mode/failure logic permits it; adjust the
conditional order/logic in useShouldSkipBasedOnMode to reference
VestTest.isSuccess, the ONE and EAGER mode checks, and the TIsolateTest failure
state so success tests will be skipped when ONE/EAGER semantics require it.
In `@website/docs/api_reference.md`:
- Around line 214-225: The docs section that lists top-level exports currently
mixes removed export memo(...) with the new success()/useSuccess() entries;
remove the memo(...) entry (or move it to a different, specific section if it
still exists elsewhere) so the top-level API docs match the actual export list
in packages/vest/src/vest.ts; update the API reference around the
success()/useSuccess() headings to delete any mention of memo(...) and run a
quick search for other occurrences of memo(...) in the API docs to ensure no
top-level references remain.
- Around line 288-297: The docs list the selector as
hasWarningByGroup(groupName) but the actual API is hasWarningsByGroup(...);
update the API reference entry to use the correct function name
hasWarningsByGroup and ensure the description matches the selector behavior
(parallel to hasErrorsByGroup/hasSuccessesByGroup); reference the real selector
name hasWarningsByGroup (as implemented alongside other selectors in
suiteSelectors.ts) so users copying the docs get the exact API.
---
Nitpick comments:
In `@packages/vest/src/testUtils/testDummy.ts`:
- Around line 65-75: createPassingSuccessAsync currently takes (name, message)
and doesn't match other async helpers' signature; change its second parameter to
an options object ({ message, time = 0 }) and inside the vi.fn async callback
call await wait(time) before invoking success() so it mirrors
createPassingAsync/createFailingAsync/createPassingWarningAsync; update
references to vestTest, TFieldName, vi.fn, success, and wait accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 474d1860-30be-4227-bd74-a7ea14f40485
⛔ Files ignored due to path filters (1)
packages/vest/src/exports/__tests__/__snapshots__/SuiteSerializer.test.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (10)
packages/vest/src/core/test/testLevelFlowControl/verifyTestRun.tspackages/vest/src/errors/ErrorStrings.tspackages/vest/src/exports/SuiteSerializer.tspackages/vest/src/hooks/__tests__/useSeverity.test.tspackages/vest/src/hooks/optional/mode.tspackages/vest/src/suiteResult/selectors/__tests__/successSelectors.test.tspackages/vest/src/suiteResult/selectors/suiteSelectors.tspackages/vest/src/testUtils/testDummy.tswebsite/docs/api_reference.mdwebsite/docs/writing_tests/success_tests.md
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/vest/src/hooks/tests/useSeverity.test.ts
ealush
changed the title
success and info severities with hooks, selectors and summary supportsuccess severity with hooks, selectors and summary support
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
packages/vest/src/hooks/__tests__/successSeverityAssertion.test.ts (2)
51-65: Same note: consider adding error assertion.Similar to the sync throw test, adding
expect(res.hasErrors('field1')).toBe(true)would confirm the test properly transitioned to an error state.💡 Optional: Add error assertion
res = suite.get(); expect(res.hasSuccesses('field1')).toBe(false); + expect(res.hasErrors('field1')).toBe(true); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/vest/src/hooks/__tests__/successSeverityAssertion.test.ts` around lines 51 - 65, Add an assertion to the async rejection test to confirm the test transitioned to an error state: after awaiting suite.run() and before the final expect(res.hasSuccesses('field1')).toBe(false), assert that res.hasErrors('field1') is true. Locate the test case (it 'Should NOT apply the message when a test rejects (async)') and add expect(res.hasErrors('field1')).toBe(true) alongside the existing use of suite.run(), vest.test(...), useSeverity(), and severity.success().
38-49: Consider adding error state assertion for completeness.The test verifies that success is not recorded, but doesn't confirm the test was recorded as an error. Adding
expect(res.hasErrors('field1')).toBe(true)would strengthen coverage.💡 Optional: Add error assertion
const res = suite.run(); expect(res.hasSuccesses('field1')).toBe(false); + expect(res.hasErrors('field1')).toBe(true); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/vest/src/hooks/__tests__/successSeverityAssertion.test.ts` around lines 38 - 49, Add an assertion that the thrown error was recorded: in the test that creates suite via vest.create and runs it (calling vest.test with useSeverity and severity.success then throwing), after the existing expect(res.hasSuccesses('field1')).toBe(false) add expect(res.hasErrors('field1')).toBe(true) so the test verifies both that a success wasn't applied and that the test is recorded as an error (use the existing res.hasErrors method).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/vest/src/core/isolate/IsolateTest/VestTest.ts`:
- Around line 267-270: The reset method currently calls
VestTest.setSeverity(test, TestSeverity.Error) before VestTest.setStatus(test,
TestAction.RESET), which lets the guard in setSeverity (that returns early when
isTested() is true) prevent clearing severity for completed tests; fix by
reversing the calls so VestTest.setStatus(test, TestAction.RESET) runs first,
then VestTest.setSeverity(...), or alternatively bypass the guard by writing
directly to test data via VestTest.setData(test, { severity: TestSeverity.Error
}) after setting status; update the VestTest.reset implementation to use one of
these approaches (referencing VestTest.reset, VestTest.setSeverity,
VestTest.setStatus, and VestTest.setData).
In `@packages/vest/src/suiteResult/selectors/useProduceSuiteSummary.ts`:
- Around line 210-228: getSummarySeverity incorrectly requires the declared
severity gate (VestTest.warns) for warnings instead of using runtime status
first, causing WARNING-status tests with Success severity to be dropped; update
getSummarySeverity to decide by runtime status: if
VestTest.isWarning(testObject).unwrap() return Severity.WARNINGS, else if
VestTest.isPassing(testObject).unwrap() &&
VestTest.isSuccess(testObject).unwrap() return Severity.SUCCESSES, else if
VestTest.isFailing(testObject).unwrap() return Severity.ERRORS (or null if none
match). Modify the logic in getSummarySeverity to check VestTest.isWarning
before using VestTest.warns so passing-success warnings are classified as
WARNINGS.
---
Nitpick comments:
In `@packages/vest/src/hooks/__tests__/successSeverityAssertion.test.ts`:
- Around line 51-65: Add an assertion to the async rejection test to confirm the
test transitioned to an error state: after awaiting suite.run() and before the
final expect(res.hasSuccesses('field1')).toBe(false), assert that
res.hasErrors('field1') is true. Locate the test case (it 'Should NOT apply the
message when a test rejects (async)') and add
expect(res.hasErrors('field1')).toBe(true) alongside the existing use of
suite.run(), vest.test(...), useSeverity(), and severity.success().
- Around line 38-49: Add an assertion that the thrown error was recorded: in the
test that creates suite via vest.create and runs it (calling vest.test with
useSeverity and severity.success then throwing), after the existing
expect(res.hasSuccesses('field1')).toBe(false) add
expect(res.hasErrors('field1')).toBe(true) so the test verifies both that a
success wasn't applied and that the test is recorded as an error (use the
existing res.hasErrors method).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 539848c7-d62c-491b-8ce7-c6b105182437
📒 Files selected for processing (9)
packages/vest/src/core/isolate/IsolateTest/VestTest.tspackages/vest/src/hooks/__tests__/successSeverityAssertion.test.tspackages/vest/src/hooks/__tests__/useSeverity.test.tspackages/vest/src/hooks/optional/mode.tspackages/vest/src/suiteResult/selectors/useProduceSuiteSummary.tspackages/vest/src/testUtils/testDummy.tswebsite/docs/api_reference.mdwebsite/static/llms-full.txtwebsite/static/llms.txt
✅ Files skipped from review due to trivial changes (2)
- website/static/llms.txt
- packages/vest/src/hooks/tests/useSeverity.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/vest/src/testUtils/testDummy.ts
- packages/vest/src/hooks/optional/mode.ts
- website/docs/api_reference.md
Comment on lines
267
to
270
| static reset(test: TIsolateTest): void { | ||
| VestTest.setSeverity(test, TestSeverity.Error); | ||
| VestTest.setStatus(test, TestAction.RESET); | ||
| } |
There was a problem hiding this comment.
Order issue: setSeverity guard may prevent reset for completed tests.
setSeverity (lines 225-227) returns early if isTested() is true (FAILED, WARNING, or PASSING status). Since setSeverity is called before setStatus(test, TestAction.RESET), the guard will fire for completed tests, leaving the stale severity unchanged.
Consider reversing the order or bypassing the guard via setData:
🐛 Proposed fix using setData directly
static reset(test: TIsolateTest): void {
- VestTest.setSeverity(test, TestSeverity.Error);
VestTest.setStatus(test, TestAction.RESET);
+ VestTest.setData(test, current => ({
+ ...current,
+ severity: TestSeverity.Error,
+ }));
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| static reset(test: TIsolateTest): void { | |
| VestTest.setSeverity(test, TestSeverity.Error); | |
| VestTest.setStatus(test, TestAction.RESET); | |
| } | |
| static reset(test: TIsolateTest): void { | |
| VestTest.setStatus(test, TestAction.RESET); | |
| VestTest.setData(test, current => ({ | |
| ...current, | |
| severity: TestSeverity.Error, | |
| })); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/vest/src/core/isolate/IsolateTest/VestTest.ts` around lines 267 -
270, The reset method currently calls VestTest.setSeverity(test,
TestSeverity.Error) before VestTest.setStatus(test, TestAction.RESET), which
lets the guard in setSeverity (that returns early when isTested() is true)
prevent clearing severity for completed tests; fix by reversing the calls so
VestTest.setStatus(test, TestAction.RESET) runs first, then
VestTest.setSeverity(...), or alternatively bypass the guard by writing directly
to test data via VestTest.setData(test, { severity: TestSeverity.Error }) after
setting status; update the VestTest.reset implementation to use one of these
approaches (referencing VestTest.reset, VestTest.setSeverity,
VestTest.setStatus, and VestTest.setData).
Comment on lines
+210
to
+228
| function getSummarySeverity(testObject: TIsolateTest): Severity | null { | ||
| const severityMatches: Record<Severity, boolean> = { | ||
| [Severity.ERRORS]: VestTest.isFailing(testObject).unwrap(), | ||
| [Severity.SUCCESSES]: | ||
| VestTest.isPassing(testObject).unwrap() && | ||
| VestTest.isSuccess(testObject).unwrap(), | ||
| [Severity.WARNINGS]: | ||
| VestTest.isWarning(testObject).unwrap() && | ||
| VestTest.warns(testObject).unwrap(), | ||
| }; | ||
|
|
||
| const severityOrder = [ | ||
| Severity.WARNINGS, | ||
| Severity.SUCCESSES, | ||
| Severity.ERRORS, | ||
| ]; | ||
|
|
||
| return severityOrder.find(severity => severityMatches[severity]) ?? null; | ||
| } |
There was a problem hiding this comment.
Warning classification now depends on the wrong dimension.
packages/vest/src/core/isolate/IsolateTest/VestTest.ts:103-141 separates runtime status (isWarning) from declared severity (warns / isSuccess). With the extra VestTest.warns(testObject) gate here, a test that ends in TestStatus.WARNING under TestSeverity.Success now returns null, so both addSummaryStats and updateFailures skip it entirely. Warnings should be classified by runtime status first, then passing successes can be special-cased.
💡 Suggested fix
function getSummarySeverity(testObject: TIsolateTest): Severity | null {
- const severityMatches: Record<Severity, boolean> = {
- [Severity.ERRORS]: VestTest.isFailing(testObject).unwrap(),
- [Severity.SUCCESSES]:
- VestTest.isPassing(testObject).unwrap() &&
- VestTest.isSuccess(testObject).unwrap(),
- [Severity.WARNINGS]:
- VestTest.isWarning(testObject).unwrap() &&
- VestTest.warns(testObject).unwrap(),
- };
-
- const severityOrder = [
- Severity.WARNINGS,
- Severity.SUCCESSES,
- Severity.ERRORS,
- ];
-
- return severityOrder.find(severity => severityMatches[severity]) ?? null;
+ if (VestTest.isWarning(testObject).unwrap()) {
+ return Severity.WARNINGS;
+ }
+
+ if (
+ VestTest.isPassing(testObject).unwrap() &&
+ VestTest.isSuccess(testObject).unwrap()
+ ) {
+ return Severity.SUCCESSES;
+ }
+
+ if (VestTest.isFailing(testObject).unwrap()) {
+ return Severity.ERRORS;
+ }
+
+ return null;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/vest/src/suiteResult/selectors/useProduceSuiteSummary.ts` around
lines 210 - 228, getSummarySeverity incorrectly requires the declared severity
gate (VestTest.warns) for warnings instead of using runtime status first,
causing WARNING-status tests with Success severity to be dropped; update
getSummarySeverity to decide by runtime status: if
VestTest.isWarning(testObject).unwrap() return Severity.WARNINGS, else if
VestTest.isPassing(testObject).unwrap() &&
VestTest.isSuccess(testObject).unwrap() return Severity.SUCCESSES, else if
VestTest.isFailing(testObject).unwrap() return Severity.ERRORS (or null if none
match). Modify the logic in getSummarySeverity to check VestTest.isWarning
before using VestTest.warns so passing-success warnings are classified as
WARNINGS.
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.
Walkthrough - Success Severity Polish & Serialization Fix
I have completed the changes requested in the PR review, including fixing the serialization bug and addressing several polish items.
Changes Made
1. Fixed Serialization Bug
severityfromDisallowedKeysand updatedstripMessagesExceptFailedAndWarningto includeTestSeverity.Success. This ensures thatsuccessmessages and thesuccessseverity level are preserved when a suite is serialized and resumed.2. PR Polish Items
SeverityMessagesas an alias forFailureMessagesin suiteSelectors.ts to provide better semantics for success-related selectors.passingSuccessAsyncto testDummy.ts for completeness.useSeverity.test.tsto reflect the removal ofinfoseverity.3. Test Refactoring
successAndInfoSelectors.test.tsto successSelectors.test.ts.getMessageintegration tests (already covered ingetFailure.test.ts).Mode.ALL, as Vest's defaultEAGERmode correctly stops field execution upon the first error.Verification Results
Automated Tests
SuiteSerializer.test.ts.Manual Verification
hasSuccessesandgetSuccesseswork as expected even when errors exist on the same field (when usingMode.ALL).severityfield andsuccessmessages.Next Steps
successseverity with hooks, selectors and summary support #1262.Summary by CodeRabbit
New Features
Tests
Documentation