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. |
|
Warning Review limit reached
More reviews will be available in 3 minutes and 17 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (23)
📝 WalkthroughWalkthroughThis PR establishes infrastructure for incrementally migrating Parse Server's test suite from JavaScript/Parse SDK to TypeScript/REST API-first testing. It adds Babel TypeScript registration, modular REST helper clients for CRUD and geo operations, comprehensive migration documentation, and converts GeoPoint/Polygon tests to REST-based test suites. ChangesTypeScript REST Test Infrastructure and GeoPoint/Polygon Migration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 5 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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.
dblythy
force-pushed
the
chore/spec-rest-polygon
branch
from
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 Sentry. 🚀 New features to boost your workflow:
|
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