test: Migrate Polygon specs to the REST client - #10485
Conversation
|
🚀 Thanks for opening this pull request! We appreciate your effort in improving the project. Please let us know once your pull request is ready for review. Tip
Note Please respond to review comments from AI agents just like you would to comments from a human reviewer. Let the reviewer resolve their own comments, unless they have reviewed and accepted your commit, or agreed with your explanation for why the feedback was incorrect. Caution Pull requests must be written using an AI agent with human supervision. Pull requests written entirely by a human will likely be rejected, because of lower code quality, higher review effort and the higher risk of introducing bugs. Please note that AI review comments on this pull request alone do not satisfy this requirement. Our CI and AI review are safeguards, not development tools. If many issues are flagged, rethink your development approach. Invest more effort in planning and design rather than using review cycles to fix low-quality code. |
📝 WalkthroughWalkthroughThis PR adds TypeScript REST test infrastructure, Jasmine runtime transpilation, typed REST and geospatial helpers, migration documentation, and REST-based GeoPoint, Polygon, and analytics test coverage while removing three legacy JavaScript test files. ChangesTypeScript REST Test Infrastructure and GeoPoint/Polygon Migration
Estimated code review effort: 3 (Moderate) | ~30 minutes Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (2 errors)
✅ Passed checks (5 passed)
✨ 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 |
Adds a narrow @babel/register hook (spec/support/tsRegister.js) loaded by Jasmine before helper.js so .ts spec/helper files are transpiled at require time. spec_files glob widened to discover .ts specs. Existing .js specs are unaffected — the hook only intercepts .ts under spec/.
Introduces spec/helpers/*.ts — a typed REST client over fetch (request, client, headers, config, errors, globals) so specs can talk to Parse Server through its public HTTP API without going through the Parse JS SDK. Also lands spec/spec_migration.md, the plan that drives the upcoming REST-first migration of the spec suite (towards parse-community#8787).
Replaces spec/Analytics.spec.js (Parse SDK + done() callbacks) with spec/server/analytics.spec.ts (REST client + async/await), plus spec/helpers/analytics.ts. No SDK import, no done(), no setTimeout. First spec to land under the new REST-first layout.
…sn't break them CI runs Node 24, which natively strips TypeScript and loads .ts as ESM, bypassing the @babel/register CJS hook. ESM needs explicit extensions, so extensionless relative imports failed. Force Jasmine's require loader (engages the hook on Node 20), add explicit .ts extensions, and mark type-only imports with 'import type' so the suite loads identically on Node 20/22/24.
Guard undefined response data in find/count helpers and fix grammar in the analytics spec description.
Use a dedicated maintenance-key value in buildHeaders, assert the Parse-error shape in expectParseError, drop the redundant local reconfigureServer decl (it lives in globals.d.ts), and fix the REST-client description in the plan.
Internal-server-error bodies use { code, message } rather than { code, error },
so a numeric code is the reliable signal that a rejection is a Parse error.
Port spec/ParseGeoPoint.spec.js to REST-based specs under spec/rest/objects (lifecycle, queries, withinPolygon) and add a geo query helper, removing the Parse JS SDK from these tests.
- Drop the misleading GeoPointLiteral[] | unknown union on withinPolygon (negative-path specs deliberately pass invalid values, so the param is unknown) - Drop the unnecessary masterKey auth in the __type response test - Filter the sub-object and array specs on a sentinel tag instead of an unfiltered find, so they don't depend on global cleanup ordering - Clarify the seed-point comments (inside / on boundary / outside)
Ports spec/ParsePolygon.spec.js to REST-based specs under spec/rest/objects/: - polygon.spec.ts: object lifecycle, equalTo, and validation - polygon-query.spec.ts: $geoIntersects point-in-polygon queries (parse-community#4608) - polygon-mongo.spec.ts: MongoDB storage format and 2d/2dsphere indexes Adds polygon/geoIntersects builders to spec/helpers/geo.ts and a describe_only_db ambient declaration. No Parse JS SDK; the storage spec uses the database adapter directly only where there is no REST surface.
8dbc778 to
27fdf42
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (4)
spec/server/analytics.spec.ts (3)
4-7: ⚡ Quick winConsider returning promises from adapter stub methods.
The stub methods return
undefinedinstead of promises. While the tests work (the controller wraps adapter calls in a promise chain), the stubs don't match the AnalyticsAdapter contract, which specifies that both methods should returnPromise.resolve({}).📝 Proposed fix to match the adapter contract
const analyticsAdapter = { - appOpened: function () {}, - trackEvent: function () {}, + appOpened: function () { + return Promise.resolve({}); + }, + trackEvent: function () { + return Promise.resolve({}); + }, };Based on learnings from Context snippet 1: AnalyticsAdapter interface specifies
appOpened(parameters, req)andtrackEvent(eventName, parameters, req)should returnPromise.resolve({}).🤖 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 `@spec/server/analytics.spec.ts` around lines 4 - 7, The analyticsAdapter stub methods currently return undefined; update the analyticsAdapter object so its appOpened and trackEvent functions return resolved promises (e.g., return Promise.resolve({}) or make them async and return {}), matching the AnalyticsAdapter contract and signatures used by the controller (ensure you modify the analyticsAdapter.appOpened and analyticsAdapter.trackEvent methods accordingly).
21-30: 💤 Low valueConsider verifying the
reqparameter is passed to the adapter.The test verifies
parameters(args[0]) but doesn't verify that the request object is passed asargs[1]. According to the AnalyticsAdapter contract,appOpened(parameters, req)receives the Express request as the second parameter.🔍 Optional: Add req parameter verification
expect(appOpenedSpy).toHaveBeenCalled(); const args = appOpenedSpy.calls.first().args; expect(args[0]).toEqual({ dimensions: { key: 'value', count: '0' } }); + expect(args[1]).toBeDefined(); // Request object is passedBased on learnings from Context snippet 1: AnalyticsAdapter interface shows
appOpened(parameters, req)receives two parameters including the HTTP request.🤖 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 `@spec/server/analytics.spec.ts` around lines 21 - 30, The test currently only asserts the adapter parameters but not that the Express request was forwarded; update the spec to also verify the second argument passed to analyticsAdapter.appOpened is the request object by inspecting appOpenedSpy.calls.first().args[1] (or using a more specific matcher) after calling reconfigureServer({ analyticsAdapter }) and appOpened(...); ensure you reference analyticsAdapter.appOpened (via appOpenedSpy) and assert args[1] is the expected req (e.g., defined or matches the test request shape) so the contract appOpened(parameters, req) is validated.
9-19: 💤 Low valueConsider verifying the
reqparameter is passed to the adapter.The test verifies
eventName(args[0]) andparameters(args[1]) but doesn't verify that the request object is passed asargs[2]. According to the AnalyticsAdapter contract,trackEvent(eventName, parameters, req)receives the Express request as the third parameter.🔍 Optional: Add req parameter verification
expect(trackSpy).toHaveBeenCalled(); const args = trackSpy.calls.first().args; expect(args[0]).toEqual('MyEvent'); expect(args[1]).toEqual({ dimensions: { key: 'value', count: '0' } }); + expect(args[2]).toBeDefined(); // Request object is passedBased on learnings from Context snippet 1: AnalyticsAdapter interface shows
trackEvent(eventName, parameters, req)receives three parameters including the HTTP request.🤖 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 `@spec/server/analytics.spec.ts` around lines 9 - 19, The test currently asserts trackEvent's eventName and parameters but doesn't verify the request is forwarded; update the spec that spies on analyticsAdapter.trackEvent (and uses track('MyEvent', ...)) to also assert that args[2] is the Express request object—e.g., add an expectation that args[2] is defined and looks like the req (has properties such as method or url) or otherwise equals the request passed into the server so trackEvent(eventName, parameters, req) is called with the req.package-lock.json (1)
68-68: No known GitHub advisories for@babel/registeret al.; bump@babel/registerfor currency.
- GitHub advisory feed shows no vulnerabilities for
@babel/register,clone-deep,pirates,pkg-dir,shallow-clone,is-plain-object, orisobject;kind-ofhas a HIGH advisory for>=6.0.0, <6.0.3, patched in6.0.3(your lock uses6.0.3).- Lockfile has
@babel/register7.27.1; latest is7.29.3(May 2026) — consider updating.🤖 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 `@package-lock.json` at line 68, Update the pinned dependency for "`@babel/register`" to a more current, non-vulnerable release (e.g., bump from 7.27.1 to the latest 7.29.3) by updating the dependency declaration in package.json (or the lockfile resolution) and regenerating the lockfile via your package manager (npm install / npm ci) so package-lock.json reflects the new "`@babel/register`" version; verify tests/builds pass and that the updated version appears in package-lock.json under the "`@babel/register`" entry.
🤖 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.
Nitpick comments:
In `@package-lock.json`:
- Line 68: Update the pinned dependency for "`@babel/register`" to a more current,
non-vulnerable release (e.g., bump from 7.27.1 to the latest 7.29.3) by updating
the dependency declaration in package.json (or the lockfile resolution) and
regenerating the lockfile via your package manager (npm install / npm ci) so
package-lock.json reflects the new "`@babel/register`" version; verify
tests/builds pass and that the updated version appears in package-lock.json
under the "`@babel/register`" entry.
In `@spec/server/analytics.spec.ts`:
- Around line 4-7: The analyticsAdapter stub methods currently return undefined;
update the analyticsAdapter object so its appOpened and trackEvent functions
return resolved promises (e.g., return Promise.resolve({}) or make them async
and return {}), matching the AnalyticsAdapter contract and signatures used by
the controller (ensure you modify the analyticsAdapter.appOpened and
analyticsAdapter.trackEvent methods accordingly).
- Around line 21-30: The test currently only asserts the adapter parameters but
not that the Express request was forwarded; update the spec to also verify the
second argument passed to analyticsAdapter.appOpened is the request object by
inspecting appOpenedSpy.calls.first().args[1] (or using a more specific matcher)
after calling reconfigureServer({ analyticsAdapter }) and appOpened(...); ensure
you reference analyticsAdapter.appOpened (via appOpenedSpy) and assert args[1]
is the expected req (e.g., defined or matches the test request shape) so the
contract appOpened(parameters, req) is validated.
- Around line 9-19: The test currently asserts trackEvent's eventName and
parameters but doesn't verify the request is forwarded; update the spec that
spies on analyticsAdapter.trackEvent (and uses track('MyEvent', ...)) to also
assert that args[2] is the Express request object—e.g., add an expectation that
args[2] is defined and looks like the req (has properties such as method or url)
or otherwise equals the request passed into the server so trackEvent(eventName,
parameters, req) is called with the req.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1ce0fb0d-0dc6-4029-bedd-9fb03a5fb47e
📒 Files selected for processing (23)
package-lock.jsonpackage.jsonspec/Analytics.spec.jsspec/ParseGeoPoint.spec.jsspec/ParsePolygon.spec.jsspec/helpers/analytics.tsspec/helpers/client.tsspec/helpers/config.tsspec/helpers/errors.tsspec/helpers/geo.tsspec/helpers/globals.d.tsspec/helpers/headers.tsspec/helpers/request.tsspec/rest/objects/geopoint-polygon.spec.tsspec/rest/objects/geopoint-query.spec.tsspec/rest/objects/geopoint.spec.tsspec/rest/objects/polygon-mongo.spec.tsspec/rest/objects/polygon-query.spec.tsspec/rest/objects/polygon.spec.tsspec/server/analytics.spec.tsspec/spec_migration.mdspec/support/jasmine.jsonspec/support/tsRegister.js
💤 Files with no reviewable changes (3)
- spec/Analytics.spec.js
- spec/ParsePolygon.spec.js
- spec/ParseGeoPoint.spec.js
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## alpha #10485 +/- ##
=======================================
Coverage 92.60% 92.60%
=======================================
Files 193 193
Lines 16893 16893
Branches 234 234
=======================================
Hits 15643 15643
Misses 1227 1227
Partials 23 23 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
spec/spec_migration.md (1)
184-185: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winResolve the TypeScript migration policy before using it as an acceptance criterion.
The checklist requires every migrated file to be renamed
.spec.ts, while the open questions still permit a.js-on-REST intermediate path. Choose one policy and make the checklist, sequencing, and acceptance criteria consistent; otherwise reviewers cannot determine whether a REST migration without TypeScript is compliant.Also applies to: 291-293
🤖 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 `@spec/spec_migration.md` around lines 184 - 185, The migration policy is inconsistent between the checklist and the open questions regarding `.spec.ts` versus a `.js`-on-REST intermediate path. Update the TypeScript migration policy, sequencing, checklist, and acceptance criteria in the migration specification so they consistently choose one supported path and clearly define whether REST migration without TypeScript is compliant.
🤖 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.
Inline comments:
In `@spec/helpers/request.ts`:
- Around line 19-22: Update ParseErrorBody and the error-handling logic in the
request helper to model parse-error payloads independently from the generic
success type T. Allow error details to be absent and support the documented
message-based internal-error shape, while preserving handling for failures
identified only by numeric code; remove casts of T to ParseErrorBody and parse
or validate the error response separately.
In `@spec/spec_migration.md`:
- Around line 82-89: Update the transpilation example in spec_migration.md to
match the tsRegister.js hook by removing .js from the documented extensions list
and retaining only .ts. Preserve the native execution path for existing
JavaScript specs.
---
Nitpick comments:
In `@spec/spec_migration.md`:
- Around line 184-185: The migration policy is inconsistent between the
checklist and the open questions regarding `.spec.ts` versus a `.js`-on-REST
intermediate path. Update the TypeScript migration policy, sequencing,
checklist, and acceptance criteria in the migration specification so they
consistently choose one supported path and clearly define whether REST migration
without TypeScript is compliant.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 761a0a70-1020-4439-9842-e33076500511
📒 Files selected for processing (23)
package-lock.jsonpackage.jsonspec/Analytics.spec.jsspec/ParseGeoPoint.spec.jsspec/ParsePolygon.spec.jsspec/helpers/analytics.tsspec/helpers/client.tsspec/helpers/config.tsspec/helpers/errors.tsspec/helpers/geo.tsspec/helpers/globals.d.tsspec/helpers/headers.tsspec/helpers/request.tsspec/rest/objects/geopoint-polygon.spec.tsspec/rest/objects/geopoint-query.spec.tsspec/rest/objects/geopoint.spec.tsspec/rest/objects/polygon-mongo.spec.tsspec/rest/objects/polygon-query.spec.tsspec/rest/objects/polygon.spec.tsspec/server/analytics.spec.tsspec/spec_migration.mdspec/support/jasmine.jsonspec/support/tsRegister.js
💤 Files with no reviewable changes (3)
- spec/Analytics.spec.js
- spec/ParseGeoPoint.spec.js
- spec/ParsePolygon.spec.js
| export interface ParseErrorBody { | ||
| code: number; | ||
| error: string; | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Model Parse-error payloads separately from success payloads.
ParseErrorBody requires error, but this helper intentionally accepts failures identified only by numeric code; the PR’s own history documents internal-error bodies using { code, message }. Casting the generic success payload T to ParseErrorBody further gives callers a false compile-time contract. Make error/message optional and parse or validate the error body independently of T. (github.com)
Also applies to: 71-78, 86-93
🤖 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 `@spec/helpers/request.ts` around lines 19 - 22, Update ParseErrorBody and the
error-handling logic in the request helper to model parse-error payloads
independently from the generic success type T. Allow error details to be absent
and support the documented message-based internal-error shape, while preserving
handling for failures identified only by numeric code; remove casts of T to
ParseErrorBody and parse or validate the error response separately.
Source: MCP tools
| - New file `spec/support/tsRegister.js` (plain JS — it is the bootstrap): | ||
| ```js | ||
| require('@babel/register')({ | ||
| extensions: ['.ts', '.js'], | ||
| only: [/[\\/]spec[\\/]/], // never transpile node_modules / lib | ||
| cache: true, | ||
| }); | ||
| ``` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Align the transpilation example with the implemented hook.
The plan documents extensions: ['.ts', '.js'], but spec/support/tsRegister.js:1-13 intentionally intercepts only .ts. Keeping .js here contradicts the stated goal that existing JavaScript specs continue running natively and could cause a future implementation to transpile the entire legacy suite.
require('`@babel/register`')({
- extensions: ['.ts', '.js'],
+ extensions: ['.ts'],
only: [/[\\/]spec[\\/]/], // never transpile node_modules / lib📝 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.
| - New file `spec/support/tsRegister.js` (plain JS — it is the bootstrap): | |
| ```js | |
| require('@babel/register')({ | |
| extensions: ['.ts', '.js'], | |
| only: [/[\\/]spec[\\/]/], // never transpile node_modules / lib | |
| cache: true, | |
| }); | |
| ``` | |
| - New file `spec/support/tsRegister.js` (plain JS — it is the bootstrap): |
🤖 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 `@spec/spec_migration.md` around lines 82 - 89, Update the transpilation
example in spec_migration.md to match the tsRegister.js hook by removing .js
from the documented extensions list and retaining only .ts. Preserve the native
execution path for existing JavaScript specs.
1 participant
Towards #8787.
Stacked on #10484 (
chore/spec-rest-geopoint) — review/merge that first; the diff here will shrink to just the Polygon files once it lands.Third slice of the REST-first spec migration:
spec/ParsePolygon.spec.jsto REST-based specs underspec/rest/objects/:polygon.spec.ts(object lifecycle,equalTo, validation),polygon-query.spec.ts($geoIntersectspoint-in-polygon queries, incl. Update test case polygons so some should fail when lat / long is used… #4608 regressions),polygon-mongo.spec.ts(MongoDB storage format + 2d/2dsphere indexes).polygon/geoIntersectsbuilders tospec/helpers/geo.tsand adescribe_only_dbambient declaration.Next: another spec migration in a follow-up PR using the same helpers.
Summary by CodeRabbit
Tests
Chores
Documentation