test: move integer settings validation coverage to component and API tests - #42048
test: move integer settings validation coverage to component and API tests#42048jessicaschelly wants to merge 3 commits into
Conversation
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (4)
💤 Files with no reviewable changes (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📜 Recent review details⏰ Context from checks skipped due to timeout. (19)
🧰 Additional context used📓 Path-based instructions (1)Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests Avoid code comments in the implementation📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc) Files:
🧠 Learnings (2)📚 Learning: 2026-02-26T19:25:44.063ZApplied to files:
📚 Learning: 2026-02-26T19:25:44.063ZApplied to files:
🔇 Additional comments (3)
WalkthroughThe changes add UI coverage for failed cleared-integer saves, update API tests for invalid numeric and null values, and remove the former settings integration test. ChangesInteger setting validation
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to This change strengthens coverage for rejected invalid integer settings, including UI error feedback and unchanged persisted values after failed bulk updates. No current merge-readiness risk is identified. Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Warning Errors were encountered while retrieving linked issues. Errors (1)
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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #42048 +/- ##
===========================================
+ Coverage 69.30% 69.42% +0.11%
===========================================
Files 4289 4291 +2
Lines 171486 171840 +354
Branches 31062 31193 +131
===========================================
+ Hits 118847 119297 +450
+ Misses 47463 47392 -71
+ Partials 5176 5151 -25
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
jessicaschelly
changed the title
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 4 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/meteor/tests/end-to-end/api/methods.ts">
<violation number="1" location="apps/meteor/tests/end-to-end/api/methods.ts:3404">
P3: These three tests only assert that a generic `error` property exists, so they pass even when the save is rejected for an unrelated reason (e.g. a permission error or a missing setting). The server rejects NaN/Infinity/-Infinity via `checkInteger` in saveSettingsBulk.ts which throws `Meteor.Error('error-invalid-setting-value', ...)`, so assert `parsedBody.error` has `error` equal to `'error-invalid-setting-value'` to actually lock in the invalid-integer contract instead of merely that some error occurred.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| }), | ||
| }) | ||
| .expect(200) | ||
| .expect(400) |
There was a problem hiding this comment.
P3: These three tests only assert that a generic error property exists, so they pass even when the save is rejected for an unrelated reason (e.g. a permission error or a missing setting). The server rejects NaN/Infinity/-Infinity via checkInteger in saveSettingsBulk.ts which throws Meteor.Error('error-invalid-setting-value', ...), so assert parsedBody.error has error equal to 'error-invalid-setting-value' to actually lock in the invalid-integer contract instead of merely that some error occurred.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/meteor/tests/end-to-end/api/methods.ts, line 3404:
<comment>These three tests only assert that a generic `error` property exists, so they pass even when the save is rejected for an unrelated reason (e.g. a permission error or a missing setting). The server rejects NaN/Infinity/-Infinity via `checkInteger` in saveSettingsBulk.ts which throws `Meteor.Error('error-invalid-setting-value', ...)`, so assert `parsedBody.error` has `error` equal to `'error-invalid-setting-value'` to actually lock in the invalid-integer contract instead of merely that some error occurred.</comment>
<file context>
@@ -3402,17 +3401,16 @@ describe('Meteor.methods', () => {
}),
})
- .expect(200)
+ .expect(400)
.expect((res) => {
- expect(res.body).to.have.property('success', true);
</file context>
There was a problem hiding this comment.
I checked this against a running server. Although saveSettingsBulk throws error-invalid-setting-value, the legacy method.call wrapper serializes the nested error as {}, so this assertion would fail. Changing that deprecated wrapper is outside this test-only PR. The new /v1/settings test covers the UI’s actual path and verifies the invalid value is rejected without changing the setting.
2 participants
Proposed changes (including videos or screenshots)
Replaces the flaky browser E2E test for empty integer settings with focused coverage at the component and API levels.
SettingsGroupPagecomponent test covering the UI flow: clear an integer input, attempt to save it, and request an error toast when the save is rejected.NaN,Infinity, and-Infinitymethod API tests by awaiting their Supertest requests, then verifies the observable failure response.settings-int.spec.ts, avoiding an unrelated dependency on loading the full Message settings page.Why the existing method tests appeared to pass
The method tests previously used
void request...inside synchronous Mocha callbacks. Because the Supertest request chains were neither returned nor awaited, Mocha completed the tests immediately without executing and enforcing their response assertions. Their passing status was therefore a false positive.Awaiting the requests exposed the actual endpoint contract: all three invalid values return HTTP
400withsuccess: falseand a DDP error frame. The legacymethod.callREST wrapper serializes the nestedMeteor.Errorobject as{}, so these tests assert only the response fields that the endpoint actually exposes. The intended empty-integer rejection and preservation of the stored setting are additionally covered through the modern settings API test.Issue(s)
FLAKY-1395
Steps to test or reproduce
yarn testapi tests/end-to-end/api/settings.ts tests/end-to-end/api/methods.ts --grep 'saveSettings|empty integer value'Further comments