Display an error message at start of test:sequential if json2capnp is…#1537
Display an error message at start of test:sequential if json2capnp is…#1537greenscientist wants to merge 1 commit intochairemobilite:mainfrom
Conversation
|
Not fully sure we want to merge it, but since I've done it, putting it here. |
WalkthroughJest configuration for sequential tests has been updated to include a global setup script. A new setup script has been added that validates the json2capnp service before running sequential tests. The script performs HTTP validation against the service, reads configuration via Preferences, handles timeouts, and provides detailed error messages with guidance on service startup commands when validation fails. Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
packages/transition-backend/jest.sequential.config.js(1 hunks)packages/transition-backend/src/api/services.socketRoutes.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-09-25T16:09:00.577Z
Learnt from: greenscientist
PR: chairemobilite/transition#1494
File: packages/transition-backend/src/services/transitRouting/TrAccessibilityMapBatch.ts:42-45
Timestamp: 2025-09-25T16:09:00.577Z
Learning: In packages/transition-backend/src/services/transitRouting/TrAccessibilityMapBatch.ts, greenscientist deferred adding input file validation using `if (!job.hasInputFile())` and requested to be reminded about it in a later PR.
Applied to files:
packages/transition-backend/src/api/services.socketRoutes.ts
📚 Learning: 2025-09-25T17:08:56.828Z
Learnt from: greenscientist
PR: chairemobilite/transition#1494
File: packages/transition-backend/src/tasks/TransitionWorkerPool.ts:105-107
Timestamp: 2025-09-25T17:08:56.828Z
Learning: In packages/transition-backend/src/tasks/TransitionWorkerPool.ts, greenscientist deferred changing the wrapBatchAccessMap input file validation from `hasInputFile()` to `getFilePath('input')` (actual file existence check), indicating it will be addressed in an upcoming file access refactor.
Applied to files:
packages/transition-backend/src/api/services.socketRoutes.ts
📚 Learning: 2025-10-20T18:31:07.612Z
Learnt from: greenscientist
PR: chairemobilite/transition#1516
File: packages/chaire-lib-backend/src/utils/trRouting/TrRoutingServiceBackend.ts:29-41
Timestamp: 2025-10-20T18:31:07.612Z
Learning: In TrRoutingServiceBackend (packages/chaire-lib-backend/src/utils/trRouting/TrRoutingServiceBackend.ts), the fetch-retry package was not working as expected, so custom retry logic was implemented instead.
Applied to files:
packages/transition-backend/src/api/services.socketRoutes.ts
📚 Learning: 2025-09-25T17:09:26.332Z
Learnt from: greenscientist
PR: chairemobilite/transition#1494
File: packages/transition-backend/src/tasks/TransitionWorkerPool.ts:85-91
Timestamp: 2025-09-25T17:09:26.332Z
Learning: In packages/transition-backend/src/tasks/TransitionWorkerPool.ts, greenscientist deferred improving input file validation to check actual file existence on disk (using getFilePath('input') instead of just hasInputFile() metadata check), indicating it should be addressed when file access is refactored soon.
Applied to files:
packages/transition-backend/src/api/services.socketRoutes.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build-and-run-UI-tests (24.x)
- GitHub Check: pr-build-check
- GitHub Check: code-lint
| options: TransitMapCalculationOptions, | ||
| callback: (status: Status.Status<TransitAccessibilityMapWithPolygonResult>) => void | ||
| ) => { | ||
| console.time('accessibiliyMap.calculateWithPolygons'); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
Performance timing label contains typo from event name.
The timing labels use "accessibiliyMap" (missing 'b'), matching the misspelled event name on line 137. While consistent, consider:
- Fixing the typo in both the event name and timing labels in a follow-up
- Using a structured logging framework instead of console.time/timeEnd for production code
Also applies to: 158-158
🏁 Script executed:
#!/bin/bash
# Check if the event name typo appears elsewhere in the codebase
rg -n "accessibiliyMap" --type ts --type js -C 2Length of output: 5560
Typo confirmed and consistently applied across codebase; fix requires coordinated changes.
The verification confirms the typo "accessibiliyMap" (missing 'b') is used consistently throughout the codebase—in event emissions (frontend), socket listeners (backend), and tests. The timing labels at lines 143 and 158 correctly match the event name, but all use the misspelled identifier.
Fixing this would require synchronized updates across packages/transition-frontend/src/services/routing/RoutingUtils.ts, packages/transition-backend/src/api/services.socketRoutes.ts, and test files to maintain frontend-backend protocol consistency. Additionally, replacing console.time/timeEnd with structured logging is recommended for production code.
🤖 Prompt for AI Agents
In packages/transition-backend/src/api/services.socketRoutes.ts around line 143,
the timing label and event identifier use the misspelled
"accessibiliyMap.calculateWithPolygons"; change the identifier to
"accessibilityMap.calculateWithPolygons" and update any
console.time/console.timeEnd calls to use the structured logger (e.g.,
processLogger.time/processLogger.info with duration) or a consistent logging
utility; then propagate the exact same identifier change to
packages/transition-frontend/src/services/routing/RoutingUtils.ts and all
affected tests so frontend, backend and tests remain in sync; if you need to
avoid an immediate breaking change, add a short-term alias that emits/listens
for both the old and new event names while logging a deprecation warning, and
remove the alias once clients are migrated.
greenscientist
force-pushed
the
warningexecintegrationtest
branch
from
0d4f51f to
644060e
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
packages/transition-backend/src/api/services.socketRoutes.ts (1)
143-143: Typo and console usage already flagged in previous review.The timing label contains the typo "accessibiliyMap" (missing 'b') and uses
console.time/timeEnd. These issues were already identified in the previous review.Also applies to: 158-158
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
packages/transition-backend/jest.sequential.config.js(1 hunks)packages/transition-backend/jest.sequential.setup.js(1 hunks)packages/transition-backend/src/api/services.socketRoutes.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: greenscientist
PR: chairemobilite/transition#1494
File: packages/transition-backend/src/services/executableJob/ExecutableJob.ts:152-158
Timestamp: 2025-09-25T16:40:55.314Z
Learning: In packages/transition-backend/src/services/executableJob/ExecutableJob.ts, greenscientist deferred improving the hasFile() method to check actual file existence on disk rather than just metadata, indicating it should be addressed in a later PR.
📚 Learning: 2025-09-25T16:09:00.577Z
Learnt from: greenscientist
PR: chairemobilite/transition#1494
File: packages/transition-backend/src/services/transitRouting/TrAccessibilityMapBatch.ts:42-45
Timestamp: 2025-09-25T16:09:00.577Z
Learning: In packages/transition-backend/src/services/transitRouting/TrAccessibilityMapBatch.ts, greenscientist deferred adding input file validation using `if (!job.hasInputFile())` and requested to be reminded about it in a later PR.
Applied to files:
packages/transition-backend/src/api/services.socketRoutes.tspackages/transition-backend/jest.sequential.config.jspackages/transition-backend/jest.sequential.setup.js
📚 Learning: 2025-10-20T18:31:07.612Z
Learnt from: greenscientist
PR: chairemobilite/transition#1516
File: packages/chaire-lib-backend/src/utils/trRouting/TrRoutingServiceBackend.ts:29-41
Timestamp: 2025-10-20T18:31:07.612Z
Learning: In TrRoutingServiceBackend (packages/chaire-lib-backend/src/utils/trRouting/TrRoutingServiceBackend.ts), the fetch-retry package was not working as expected, so custom retry logic was implemented instead.
Applied to files:
packages/transition-backend/src/api/services.socketRoutes.ts
📚 Learning: 2025-09-25T17:08:56.828Z
Learnt from: greenscientist
PR: chairemobilite/transition#1494
File: packages/transition-backend/src/tasks/TransitionWorkerPool.ts:105-107
Timestamp: 2025-09-25T17:08:56.828Z
Learning: In packages/transition-backend/src/tasks/TransitionWorkerPool.ts, greenscientist deferred changing the wrapBatchAccessMap input file validation from `hasInputFile()` to `getFilePath('input')` (actual file existence check), indicating it will be addressed in an upcoming file access refactor.
Applied to files:
packages/transition-backend/src/api/services.socketRoutes.tspackages/transition-backend/jest.sequential.config.jspackages/transition-backend/jest.sequential.setup.js
📚 Learning: 2025-09-25T17:09:26.332Z
Learnt from: greenscientist
PR: chairemobilite/transition#1494
File: packages/transition-backend/src/tasks/TransitionWorkerPool.ts:85-91
Timestamp: 2025-09-25T17:09:26.332Z
Learning: In packages/transition-backend/src/tasks/TransitionWorkerPool.ts, greenscientist deferred improving input file validation to check actual file existence on disk (using getFilePath('input') instead of just hasInputFile() metadata check), indicating it should be addressed when file access is refactored soon.
Applied to files:
packages/transition-backend/src/api/services.socketRoutes.tspackages/transition-backend/jest.sequential.config.jspackages/transition-backend/jest.sequential.setup.js
📚 Learning: 2025-09-25T16:40:55.314Z
Learnt from: greenscientist
PR: chairemobilite/transition#1494
File: packages/transition-backend/src/services/executableJob/ExecutableJob.ts:152-158
Timestamp: 2025-09-25T16:40:55.314Z
Learning: In packages/transition-backend/src/services/executableJob/ExecutableJob.ts, greenscientist deferred improving the hasFile() method to check actual file existence on disk rather than just metadata, indicating it should be addressed in a later PR.
Applied to files:
packages/transition-backend/jest.sequential.config.jspackages/transition-backend/jest.sequential.setup.js
🧬 Code graph analysis (1)
packages/transition-backend/jest.sequential.setup.js (1)
packages/chaire-lib-backend/src/utils/trRouting/TrRoutingServiceBackend.ts (1)
request(21-45)
🪛 GitHub Check: code-lint
packages/transition-backend/jest.sequential.setup.js
[failure] 52-52:
'err' is defined but never used. Allowed unused args must match /^_/u
[failure] 47-47:
'res' is defined but never used. Allowed unused args must match /^_/u
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: pr-build-check
- GitHub Check: build-and-run-UI-tests (24.x)
- GitHub Check: build-and-test (24.x)
🔇 Additional comments (2)
packages/transition-backend/jest.sequential.setup.js (1)
12-24: Clear error messaging with helpful guidance.The error display function provides excellent developer experience with specific instructions to start the service.
packages/transition-backend/jest.sequential.config.js (1)
19-20: Configuration correctly wires the global setup.The
globalSetupproperty now references the Jest setup script provided in this PR. Ensure the issues identified injest.sequential.setup.jsare addressed.
| const host = json2CapnpConfig.host.replace(/^https?:\/\//, ''); | ||
| const port = json2CapnpConfig.port; |
There was a problem hiding this comment.
Handle https protocol in addition to http.
The code strips the protocol and always uses the http module (line 42), but the configuration might specify https://localhost. This could cause connection failures if the service is configured for HTTPS.
Consider detecting the protocol and using the appropriate module:
- const host = json2CapnpConfig.host.replace(/^https?:\/\//, '');
- const port = json2CapnpConfig.port;
+ const isHttps = json2CapnpConfig.host.startsWith('https://');
+ const host = json2CapnpConfig.host.replace(/^https?:\/\//, '');
+ const port = json2CapnpConfig.port;
+ const httpModule = isHttps ? require('https') : require('http');Then use httpModule.get instead of http.get on line 42:
+module.exports = async () => {
+ const Preferences = require('chaire-lib-common/lib/config/Preferences').default;
+
+ // Get json2capnp configuration
+ const json2CapnpConfig = Preferences.get('json2Capnp', {
+ host: 'http://localhost',
+ port: 2000
+ });
+
+ const isHttps = json2CapnpConfig.host.startsWith('https://');
+ const host = json2CapnpConfig.host.replace(/^https?:\/\//, '');
+ const port = json2CapnpConfig.port;
+ const httpModule = isHttps ? require('https') : require('http');- const request = http.get({
+ const request = httpModule.get({Committable suggestion skipped: line range outside the PR's diff.
| port: port, | ||
| path: '/', | ||
| timeout: 2000 | ||
| }, (res) => { |
There was a problem hiding this comment.
Prefix unused parameters with underscore.
ESLint requires unused parameters to be prefixed with underscore.
Apply this diff:
- }, (res) => {
+ }, (_res) => {
console.log('✓ json2capnp service is running');
resolve();
});
- request.on('error', (err) => {
+ request.on('error', (_err) => {
displayJson2CapnpError(host, port, 'not running');
reject(new Error(`json2capnp service is not available at ${host}:${port}`));
});Also applies to: 52-52
🧰 Tools
🪛 GitHub Check: code-lint
[failure] 47-47:
'res' is defined but never used. Allowed unused args must match /^_/u
🤖 Prompt for AI Agents
In packages/transition-backend/jest.sequential.setup.js around lines 47 and 52,
ESLint flags unused callback parameters; update the function parameter names by
prefixing them with an underscore (e.g., change res to _res and err to _err or
similar) so they are marked as intentionally unused and satisfy the lint rule,
keeping the rest of the callback logic unchanged.
… not running This will display that error message: ================================================================================ ERROR: json2capnp service is not running! ================================================================================ The sequential tests require the json2capnp rust server to be started. To start the service, run the following commands: cd services/json2capnp cargo run 2000 ../../projects/test/test_cache/test Or in a single command from the project root: (cd services/json2capnp && cargo run 2000 ../../projects/test/test_cache/test) Expected service location: localhost:2000 ================================================================================
greenscientist
force-pushed
the
warningexecintegrationtest
branch
from
644060e to
7005ec9
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
packages/transition-backend/jest.sequential.setup.js (3)
36-37: HTTPS protocol not supported.The code strips the protocol but always uses the
httpmodule (line 42), which will fail if the configuration specifieshttps://.
47-50: Prefix unused parameter with underscore.ESLint requires unused parameters to be prefixed with underscore.
Apply this diff:
- }, (res) => { + }, (_res) => { console.log('✓ json2capnp service is running'); resolve();
52-55: Prefix unused parameter with underscore.ESLint requires unused parameters to be prefixed with underscore.
Apply this diff:
- request.on('error', (err) => { + request.on('error', (_err) => { displayJson2CapnpError(host, port, 'not running'); reject(new Error(`json2capnp service is not available at ${host}:${port}`));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
packages/transition-backend/jest.sequential.config.js(1 hunks)packages/transition-backend/jest.sequential.setup.js(1 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: greenscientist
PR: chairemobilite/transition#1494
File: packages/transition-backend/src/services/executableJob/ExecutableJob.ts:152-158
Timestamp: 2025-09-25T16:40:55.314Z
Learning: In packages/transition-backend/src/services/executableJob/ExecutableJob.ts, greenscientist deferred improving the hasFile() method to check actual file existence on disk rather than just metadata, indicating it should be addressed in a later PR.
📚 Learning: 2025-09-25T17:09:26.332Z
Learnt from: greenscientist
PR: chairemobilite/transition#1494
File: packages/transition-backend/src/tasks/TransitionWorkerPool.ts:85-91
Timestamp: 2025-09-25T17:09:26.332Z
Learning: In packages/transition-backend/src/tasks/TransitionWorkerPool.ts, greenscientist deferred improving input file validation to check actual file existence on disk (using getFilePath('input') instead of just hasInputFile() metadata check), indicating it should be addressed when file access is refactored soon.
Applied to files:
packages/transition-backend/jest.sequential.setup.jspackages/transition-backend/jest.sequential.config.js
📚 Learning: 2025-09-25T17:08:56.828Z
Learnt from: greenscientist
PR: chairemobilite/transition#1494
File: packages/transition-backend/src/tasks/TransitionWorkerPool.ts:105-107
Timestamp: 2025-09-25T17:08:56.828Z
Learning: In packages/transition-backend/src/tasks/TransitionWorkerPool.ts, greenscientist deferred changing the wrapBatchAccessMap input file validation from `hasInputFile()` to `getFilePath('input')` (actual file existence check), indicating it will be addressed in an upcoming file access refactor.
Applied to files:
packages/transition-backend/jest.sequential.setup.jspackages/transition-backend/jest.sequential.config.js
📚 Learning: 2025-09-25T16:09:00.577Z
Learnt from: greenscientist
PR: chairemobilite/transition#1494
File: packages/transition-backend/src/services/transitRouting/TrAccessibilityMapBatch.ts:42-45
Timestamp: 2025-09-25T16:09:00.577Z
Learning: In packages/transition-backend/src/services/transitRouting/TrAccessibilityMapBatch.ts, greenscientist deferred adding input file validation using `if (!job.hasInputFile())` and requested to be reminded about it in a later PR.
Applied to files:
packages/transition-backend/jest.sequential.setup.jspackages/transition-backend/jest.sequential.config.js
📚 Learning: 2025-09-25T16:40:55.314Z
Learnt from: greenscientist
PR: chairemobilite/transition#1494
File: packages/transition-backend/src/services/executableJob/ExecutableJob.ts:152-158
Timestamp: 2025-09-25T16:40:55.314Z
Learning: In packages/transition-backend/src/services/executableJob/ExecutableJob.ts, greenscientist deferred improving the hasFile() method to check actual file existence on disk rather than just metadata, indicating it should be addressed in a later PR.
Applied to files:
packages/transition-backend/jest.sequential.config.js
🪛 GitHub Check: code-lint
packages/transition-backend/jest.sequential.setup.js
[failure] 52-52:
'err' is defined but never used. Allowed unused args must match /^_/u
[failure] 47-47:
'res' is defined but never used. Allowed unused args must match /^_/u
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build-and-test (24.x)
- GitHub Check: build-and-run-UI-tests (24.x)
- GitHub Check: pr-build-check
🔇 Additional comments (2)
packages/transition-backend/jest.sequential.setup.js (1)
12-24: Clear and helpful error messaging.The error display function provides actionable guidance with specific commands for starting the json2capnp service.
packages/transition-backend/jest.sequential.config.js (1)
19-20: Configuration correctly references the new setup file.The globalSetup property properly points to the setup script that validates the json2capnp service. The trailing comma on line 19 follows JavaScript best practices.
| }, (res) => { | ||
| console.log('✓ json2capnp service is running'); | ||
| resolve(); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider validating the response status code.
Currently any HTTP response is treated as success, including error status codes like 404 or 500. The service might be running but unhealthy.
Apply this diff to check the status:
- }, (_res) => {
+ }, (res) => {
+ if (res.statusCode !== 200) {
+ displayJson2CapnpError(host, port, `returned status ${res.statusCode}`);
+ reject(new Error(`json2capnp service returned status ${res.statusCode}`));
+ return;
+ }
console.log('✓ json2capnp service is running');
resolve();Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 GitHub Check: code-lint
[failure] 47-47:
'res' is defined but never used. Allowed unused args must match /^_/u
🤖 Prompt for AI Agents
In packages/transition-backend/jest.sequential.setup.js around lines 47 to 50,
the current callback treats any HTTP response as success; change it to validate
the response status code by checking res.statusCode (or res.status) and only
call resolve() for 2xx responses, otherwise log the status and response body (or
status message) and call reject(new Error(...)) so non-2xx responses (e.g.,
404/500) fail the setup; ensure the request/response is properly consumed/closed
when rejecting.
tahini
left a comment
tahini
left a comment
There was a problem hiding this comment.
Je ne sais pas si on veut ça dans un globalSetup ou plutôt dans un beforeAll des tests qui le nécessitent (quitte à appeler un helper).
Dans mon flot habituel, je roule uniquement le test DB qui m'intéresse et donc d'avoir ça tout le temps est un peu inutile, mais pas dommageable non plus...
2 participants
… not running
This will display that error message:
================================================================================ ERROR: json2capnp service is not running!
The sequential tests require the json2capnp rust server to be started.
To start the service, run the following commands:
cd services/json2capnp
cargo run 2000 ../../projects/test/test_cache/test
Or in a single command from the project root:
(cd services/json2capnp && cargo run 2000 ../../projects/test/test_cache/test)
Expected service location: localhost:2000
================================================================================
Summary by CodeRabbit
Tests