feat(be): extend departure endpoint #35
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Skipped Deployment
|
|
Warning Rate limit exceeded@krystxf has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 4 minutes and 34 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, 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 have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThe pull request introduces modifications to the Changes
Possibly related PRs
Suggested labels
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Outside diff range and nitpick comments (1)
apps/backend/package.json (1)
Line range hint
67-69: Move development dependencies to devDependencies section.
dotenv-cliandprismaare development tools and should be indevDependenciesrather thanoptionalDependencies.Apply this change:
"devDependencies": { + "dotenv-cli": "^7.4.2", + "prisma": "^5.20.0", "@nestjs/cli": "^10.4.5", // ... other dev dependencies }, "optionalDependencies": { - "dotenv-cli": "^7.4.2", - "prisma": "^5.20.0" },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- apps/backend/package.json (1 hunks)
- apps/backend/src/modules/departure/departure.controller.ts (3 hunks)
- apps/backend/src/modules/departure/departure.service.ts (2 hunks)
🔇 Additional comments (3)
apps/backend/package.json (2)
Line range hint
15-16: Consider updating e2e test configuration for new features.The PR mentions planned e2e tests for the new departure endpoint features. Consider updating the test configuration and adding specific test scripts for the new endpoint functionality.
Let's check the current e2e test setup:
#!/bin/bash # Check existing e2e tests for departure endpoint if [ -d "e2e" ]; then echo "Checking existing e2e tests..." rg -l "departure" e2e/ else echo "Warning: e2e directory not found" fiWould you like help setting up e2e tests for the new departure endpoint features?
Line range hint
19-24: LGTM! Environment variable handling improvement.The addition of
dotenv -e .env.localto Prisma commands ensures consistent environment variable loading during development.Let's verify the environment configuration:
✅ Verification successful
The previous verification attempt showed that .env.local is not present in the sandbox environment, which is expected. Let's analyze the package.json configuration and look for any environment file templates to understand the expected setup better.
The output shows that
dotenv-cliandprismaare in optionalDependencies, and there are example env files available. Let's examine these files and documentation to better understand the environment setup before making a final assessment about the review comment's concerns.
Environment configuration and dependencies are properly set up
The review comment's concerns can be addressed based on the gathered evidence:
- Using
.env.localfor development is correct and documented in the setup instructions.- Both
.env.local.exampleand.env.docker.exampleprovide proper templates with all required configurations.- Having
dotenv-cliandprismain optionalDependencies is acceptable as:
- They are development tools but may not be required in all environments
- The setup is well-documented and works with Docker and local development
- The presence of example env files and clear setup instructions mitigates any potential issues
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if .env.local exists and contains required database configuration if [ -f ".env.local" ]; then echo "Found .env.local file" # Safely check for DATABASE_URL without displaying it if grep -q "DATABASE_URL=" .env.local; then echo "DATABASE_URL configuration found" else echo "Warning: DATABASE_URL not found in .env.local" fi else echo "Warning: .env.local file not found" fiLength of output: 418
Script:
#!/bin/bash # Let's check the package.json for environment-related configurations and scripts cat apps/backend/package.json | jq '.scripts + { dependencies: .dependencies, devDependencies: .devDependencies, optionalDependencies: .optionalDependencies }' # Check if there's any example env file or documentation fd -t f -e example -e sample -e template envLength of output: 2756
Script:
#!/bin/bash # Let's check the example env files to understand the expected configuration cat apps/backend/.env.local.example cat .env.docker.example # Also check if there's any documentation about environment setup fd README -t f | xargs catLength of output: 2946
apps/backend/src/modules/departure/departure.service.ts (1)
2-2: LGTM! Good choice of utility library.The
radashlibrary is a good choice for array deduplication, offering better performance than Set-based alternatives.
| async getDepartures(args: { | ||
| stopIds: string[]; | ||
| platformIds: string[]; | ||
| metroOnly: boolean; | ||
| }): Promise<DepartureSchema[]> { |
There was a problem hiding this comment.
Add input validation for empty arrays.
Consider validating the input arrays to ensure at least one of stopIds or platformIds is non-empty, as the current implementation might make an unnecessary database query with empty arrays.
async getDepartures(args: {
stopIds: string[];
platformIds: string[];
metroOnly: boolean;
}): Promise<DepartureSchema[]> {
+ if (args.stopIds.length === 0 && args.platformIds.length === 0) {
+ throw new HttpException(
+ 'At least one stopId or platformId must be provided',
+ HttpStatus.BAD_REQUEST
+ );
+ }📝 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.
| async getDepartures(args: { | |
| stopIds: string[]; | |
| platformIds: string[]; | |
| metroOnly: boolean; | |
| }): Promise<DepartureSchema[]> { | |
| async getDepartures(args: { | |
| stopIds: string[]; | |
| platformIds: string[]; | |
| metroOnly: boolean; | |
| }): Promise<DepartureSchema[]> { | |
| if (args.stopIds.length === 0 && args.platformIds.length === 0) { | |
| throw new HttpException( | |
| 'At least one stopId or platformId must be provided', | |
| HttpStatus.BAD_REQUEST | |
| ); | |
| } |
| const parsedDepartures = parsed.data.departures.map((departure) => { | ||
| return { | ||
| departure: departure.departure_timestamp, | ||
| delay: getDelayInSeconds(departure.delay), | ||
| headsign: departure.trip.headsign, | ||
| route: departure.route.short_name, | ||
| platformId: departure.stop.id, | ||
| platformCode: departure.stop.platform_code, | ||
| }; | ||
| }); | ||
|
|
||
| return parsedDepartures; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Extract common departure parsing logic.
The departure parsing logic is duplicated between getDepartures and getDeparturesByPlatform. Consider extracting this into a private method.
+private parseDepartureResponse(data: unknown): DepartureSchema[] {
+ const parsed = departureBoardsSchema.safeParse(data);
+ if (!parsed.success) {
+ throw new Error(parsed.error.message);
+ }
+ return parsed.data.departures.map((departure) => ({
+ departure: departure.departure_timestamp,
+ delay: getDelayInSeconds(departure.delay),
+ headsign: departure.trip.headsign,
+ route: departure.route.short_name,
+ platformId: departure.stop.id,
+ platformCode: departure.stop.platform_code,
+ }));
+}Committable suggestion was skipped due to low confidence.
| const dbPlatforms = ( | ||
| await this.prisma.platform.findMany({ | ||
| select: { id: true }, | ||
| where: { | ||
| id: { in: args.platformIds }, | ||
| ...(args.metroOnly ? { isMetro: true } : {}), | ||
| }, | ||
| }) | ||
| ).map((platform) => platform.id); | ||
| const stopPlatforms = ( | ||
| await this.prisma.stop.findMany({ | ||
| select: { | ||
| platforms: { | ||
| select: { id: true }, | ||
| where: { ...(args.metroOnly ? { isMetro: true } : {}) }, | ||
| }, | ||
| }, | ||
| where: { id: { in: args.stopIds } }, | ||
| }) | ||
| ).flatMap((stop) => stop.platforms.map((platform) => platform.id)); | ||
| const allPlatformIds = unique([...dbPlatforms, ...stopPlatforms]).slice( | ||
| 0, | ||
| 100, | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling and extract magic numbers.
Several improvements could be made to this section:
- Add error handling for database queries
- Extract the limit constant
- Consider combining the queries for better performance
+const MAX_PLATFORMS = 100; // Maximum number of platforms to query at once due to API limitations
+
async getDepartures(args: {
stopIds: string[];
platformIds: string[];
metroOnly: boolean;
}): Promise<DepartureSchema[]> {
+ try {
const dbPlatforms = (
await this.prisma.platform.findMany({
select: { id: true },
where: {
id: { in: args.platformIds },
...(args.metroOnly ? { isMetro: true } : {}),
},
})
).map((platform) => platform.id);
// ... rest of the code ...
- ).slice(0, 100);
+ ).slice(0, MAX_PLATFORMS);
+ } catch (error) {
+ throw new HttpException(
+ 'Failed to fetch platform data',
+ HttpStatus.INTERNAL_SERVER_ERROR
+ );
+ }Committable suggestion was skipped due to low confidence.
| const searchParams = new URLSearchParams( | ||
| allPlatformIds | ||
| .map((id) => ["ids", id]) | ||
| .concat([ | ||
| ["skip", "canceled"], | ||
| ["mode", "departures"], | ||
| ["order", "real"], | ||
| ]), | ||
| ); | ||
|
|
||
| const url = new URL( | ||
| `${GOLEMIO_API}/v2/pid/departureboards?${searchParams.toString()}`, | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Extract API query parameters as constants.
Consider extracting the API query parameters to improve maintainability and reusability, especially since these parameters are also used in getDeparturesByPlatform.
+const DEPARTURE_API_PARAMS = [
+ ['skip', 'canceled'],
+ ['mode', 'departures'],
+ ['order', 'real'],
+] as const;
+
const searchParams = new URLSearchParams(
allPlatformIds
.map((id) => ["ids", id])
- .concat([
- ["skip", "canceled"],
- ["mode", "departures"],
- ["order", "real"],
- ]),
+ .concat(DEPARTURE_API_PARAMS),
);📝 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.
| const searchParams = new URLSearchParams( | |
| allPlatformIds | |
| .map((id) => ["ids", id]) | |
| .concat([ | |
| ["skip", "canceled"], | |
| ["mode", "departures"], | |
| ["order", "real"], | |
| ]), | |
| ); | |
| const url = new URL( | |
| `${GOLEMIO_API}/v2/pid/departureboards?${searchParams.toString()}`, | |
| ); | |
| const DEPARTURE_API_PARAMS = [ | |
| ['skip', 'canceled'], | |
| ['mode', 'departures'], | |
| ['order', 'real'], | |
| ] as const; | |
| const searchParams = new URLSearchParams( | |
| allPlatformIds | |
| .map((id) => ["ids", id]) | |
| .concat(DEPARTURE_API_PARAMS), | |
| ); | |
| const url = new URL( | |
| `${GOLEMIO_API}/v2/pid/departureboards?${searchParams.toString()}`, | |
| ); |
| const res = await fetch(url, { | ||
| method: "GET", | ||
| headers: { | ||
| "Content-Type": "application/json", | ||
| "X-Access-Token": process.env.GOLEMIO_API_KEY ?? "", | ||
| }, | ||
| }); | ||
| if (!res.ok) { | ||
| throw new Error( | ||
| `Failed to fetch departure data: ${res.status} ${res.statusText}`, | ||
| ); | ||
| } | ||
|
|
||
| const json = await res.json(); | ||
| const parsed = departureBoardsSchema.safeParse(json); | ||
|
|
||
| if (!parsed.success) { | ||
| throw new Error(parsed.error.message); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance API call robustness.
The API call could benefit from several improvements:
- Add timeout handling
- Implement retry mechanism for transient failures
- Validate API key presence at service initialization
+private validateApiKey() {
+ if (!process.env.GOLEMIO_API_KEY) {
+ throw new Error('GOLEMIO_API_KEY environment variable is not set');
+ }
+}
+
+constructor(private prisma: PrismaService) {
+ this.validateApiKey();
+}
+
+private async fetchWithTimeout(url: URL, options: RequestInit, timeout = 5000) {
+ const controller = new AbortController();
+ const id = setTimeout(() => controller.abort(), timeout);
+ try {
+ const response = await fetch(url, {
+ ...options,
+ signal: controller.signal,
+ });
+ clearTimeout(id);
+ return response;
+ } catch (error) {
+ clearTimeout(id);
+ throw error;
+ }
+}Committable suggestion was skipped due to low confidence.
| async getDepartures(args: { | ||
| stopIds: string[]; | ||
| platformIds: string[]; | ||
| metroOnly: boolean; | ||
| }): Promise<DepartureSchema[]> { | ||
| const dbPlatforms = ( | ||
| await this.prisma.platform.findMany({ | ||
| select: { id: true }, | ||
| where: { | ||
| id: { in: args.platformIds }, | ||
| ...(args.metroOnly ? { isMetro: true } : {}), | ||
| }, | ||
| }) | ||
| ).map((platform) => platform.id); | ||
| const stopPlatforms = ( | ||
| await this.prisma.stop.findMany({ | ||
| select: { | ||
| platforms: { | ||
| select: { id: true }, | ||
| where: { ...(args.metroOnly ? { isMetro: true } : {}) }, | ||
| }, | ||
| }, | ||
| where: { id: { in: args.stopIds } }, | ||
| }) | ||
| ).flatMap((stop) => stop.platforms.map((platform) => platform.id)); | ||
| const allPlatformIds = unique([...dbPlatforms, ...stopPlatforms]).slice( | ||
| 0, | ||
| 100, | ||
| ); | ||
|
|
||
| const searchParams = new URLSearchParams( | ||
| allPlatformIds | ||
| .map((id) => ["ids", id]) | ||
| .concat([ | ||
| ["skip", "canceled"], | ||
| ["mode", "departures"], | ||
| ["order", "real"], | ||
| ]), | ||
| ); | ||
|
|
||
| const url = new URL( | ||
| `${GOLEMIO_API}/v2/pid/departureboards?${searchParams.toString()}`, | ||
| ); | ||
|
|
||
| const res = await fetch(url, { | ||
| method: "GET", | ||
| headers: { | ||
| "Content-Type": "application/json", | ||
| "X-Access-Token": process.env.GOLEMIO_API_KEY ?? "", | ||
| }, | ||
| }); | ||
| if (!res.ok) { | ||
| throw new Error( | ||
| `Failed to fetch departure data: ${res.status} ${res.statusText}`, | ||
| ); | ||
| } | ||
|
|
||
| const json = await res.json(); | ||
| const parsed = departureBoardsSchema.safeParse(json); | ||
|
|
||
| if (!parsed.success) { | ||
| throw new Error(parsed.error.message); | ||
| } | ||
|
|
||
| const parsedDepartures = parsed.data.departures.map((departure) => { | ||
| return { | ||
| departure: departure.departure_timestamp, | ||
| delay: getDelayInSeconds(departure.delay), | ||
| headsign: departure.trip.headsign, | ||
| route: departure.route.short_name, | ||
| platformId: departure.stop.id, | ||
| platformCode: departure.stop.platform_code, | ||
| }; | ||
| }); | ||
|
|
||
| return parsedDepartures; | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
E2e tests need to be updated for new endpoint functionality
The existing e2e tests only cover the basic platform ID scenario and need to be extended to test:
- Multiple platform IDs
- Stop IDs functionality
- Metro-only filter
- Combination of different parameters
Current test file location: apps/backend/e2e/tests/departure.e2e-spec.ts
🔗 Analysis chain
Reminder: Add e2e tests as mentioned in PR objectives.
The implementation successfully adds support for:
- Retrieving departures by stop IDs
- Retrieving departures by platform IDs
- Filtering departures with metroOnly parameter
However, e2e tests were mentioned in the PR objectives but haven't been implemented yet.
Would you like help creating e2e tests for the new functionality?
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if e2e tests exist for the departure endpoint
fd -e spec.ts -e test.ts departure apps/backend/test/e2e
Length of output: 167
Script:
#!/bin/bash
# Let's first check if there are any e2e test directories in the backend
fd -t d -g "*e2e*" apps/backend
# Also search for any test files related to departures in the backend
fd -e spec.ts -e test.ts departure apps/backend
Length of output: 101
Script:
#!/bin/bash
# Let's check the content of e2e directory to understand the test structure
ls -la apps/backend/e2e/
# Also check if there are any departure-related files that might have a different naming pattern
rg -l "departure" apps/backend/e2e/
Length of output: 332
Script:
#!/bin/bash
# Let's check the content of the departure e2e test file
cat apps/backend/e2e/tests/departure.e2e-spec.ts
Length of output: 1736
| throw new HttpException( | ||
| "Invalid query params", | ||
| HttpStatus.BAD_REQUEST, | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Include validation errors in the HttpException for better debugging
Currently, when validation fails, a generic error message is returned. Including detailed validation errors can help clients understand what went wrong with their request.
Apply this diff to include validation details in the exception:
if (!parsed.success) {
throw new HttpException(
- "Invalid query params",
+ parsed.error.errors,
HttpStatus.BAD_REQUEST,
);
}This change will return the specific validation errors encountered during parsing.
Committable suggestion was skipped due to low confidence.
| name: "platform[]", | ||
| description: "Platform IDs", | ||
| type: String, | ||
| isArray: true, | ||
| allowEmptyValue: true, | ||
| required: false, | ||
| }, | ||
| { | ||
| name: "stop[]", | ||
| description: "Stop IDs", | ||
| type: String, | ||
| isArray: true, | ||
| allowEmptyValue: true, | ||
| required: false, | ||
| }, |
There was a problem hiding this comment.
Remove '[]' from query parameter names to avoid parsing issues
Using brackets in the query parameter names ("platform[]" and "stop[]") can lead to unexpected behavior in parameter parsing. It's standard practice to name the parameters without brackets when using isArray: true.
Apply this diff to correct the parameter names and improve consistency:
{
- name: "platform[]",
+ name: "platform",
description: "Platform IDs",
type: String,
isArray: true,
- allowEmptyValue: true,
+ allowEmptyValue: false,
required: false,
},
{
- name: "stop[]",
+ name: "stop",
description: "Stop IDs",
type: String,
isArray: true,
- allowEmptyValue: true,
+ allowEmptyValue: false,
required: false,
},📝 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.
| name: "platform[]", | |
| description: "Platform IDs", | |
| type: String, | |
| isArray: true, | |
| allowEmptyValue: true, | |
| required: false, | |
| }, | |
| { | |
| name: "stop[]", | |
| description: "Stop IDs", | |
| type: String, | |
| isArray: true, | |
| allowEmptyValue: true, | |
| required: false, | |
| }, | |
| name: "platform", | |
| description: "Platform IDs", | |
| type: String, | |
| isArray: true, | |
| allowEmptyValue: false, | |
| required: false, | |
| }, | |
| { | |
| name: "stop", | |
| description: "Stop IDs", | |
| type: String, | |
| isArray: true, | |
| allowEmptyValue: false, | |
| required: false, | |
| }, |
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Outside diff range and nitpick comments (6)
apps/backend/src/modules/status/status.types.ts (1)
1-5: LGTM! Consider adding JSDoc comments.The enum is well-structured and follows naming conventions. Consider adding JSDoc comments to document the purpose of each service status.
export enum SystemStatusService { + /** Status of the main backend service */ BACKEND = "backend", + /** Status of the geolocation functions */ GEO_FUNCTIONS = "geo-functions", + /** Status of the database data integrity */ DB_DATA = "db-data", }apps/backend/src/modules/status/status.module.ts (1)
6-11: Add module documentation to clarify its purpose.Consider adding JSDoc documentation to describe the module's purpose and responsibilities, especially since it's related to system status functionality.
Add documentation above the @module decorator:
+/** + * StatusModule handles system status-related functionality. + * This module provides endpoints for checking the backend's operational status + * and health monitoring capabilities. + */ @Module({ controllers: [StatusController], providers: [StatusService], imports: [], })apps/backend/src/modules/prisma/prisma.service.ts (1)
23-36: Consider adding unit tests for database extension methods.Since these methods are critical for geo functionality and the PR mentions pending e2e tests, we should ensure proper test coverage.
Would you like me to help create unit tests for these methods? We can mock the Prisma client and test various scenarios including:
- Successful extension retrieval
- Database connection failures
- Empty/invalid extension data
apps/backend/src/app.module.ts (1)
27-27: Consider module dependencies and ordering.While the current implementation works, consider:
- If
StatusModuleneeds to monitor other services, it might need to be registered after them- Explicit ordering (infrastructure → core → feature modules) could improve maintainability
Consider grouping the imports as follows:
@Module({ imports: [ + // Infrastructure modules + ConfigModule.forRoot(configModuleConfig), + ScheduleModule.forRoot(), + CacheModule.registerAsync(cacheModuleConfig), + GraphQLModule.forRoot<ApolloDriverConfig>({ + driver: ApolloDriver, + playground: true, + autoSchemaFile: "schema.gql", + path: GRAPHQL_API_ROOT, + }), + // Core modules + PrismaModule, + LoggerModule, + // Feature modules PlatformModule, DepartureModule, ImportModule, StopModule, - PrismaModule, - LoggerModule, StatusModule, - ConfigModule.forRoot(configModuleConfig), - ScheduleModule.forRoot(), - CacheModule.registerAsync(cacheModuleConfig), - GraphQLModule.forRoot<ApolloDriverConfig>({ - driver: ApolloDriver, - playground: true, - autoSchemaFile: "schema.gql", - path: GRAPHQL_API_ROOT, - }), ],apps/backend/src/modules/status/status.controller.ts (2)
1-15: Consider grouping related imports together.For better organization, consider grouping imports into these categories:
- External dependencies (@nestjs)
- Internal decorators
- Internal modules/services
import { Controller, Get } from "@nestjs/common"; import { ApiResponse, ApiTags } from "@nestjs/swagger"; import { ApiDescription } from "src/decorators/swagger.decorator"; -import { StatusService } from "src/modules/status/status.service"; import { StatusObject, SystemStatus, SystemStatusService, } from "src/modules/status/status.types"; +import { StatusService } from "src/modules/status/status.service";
12-14: Consider adding rate limiting and caching for status endpoints.As these are health check endpoints, consider implementing:
- Rate limiting to prevent potential DoS attacks
- Response caching with a short TTL (e.g., 30 seconds) to reduce system load
- Integration with monitoring systems (e.g., Prometheus metrics)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
- apps/backend/src/app.module.ts (2 hunks)
- apps/backend/src/modules/prisma/prisma.service.ts (1 hunks)
- apps/backend/src/modules/status/status.controller.ts (1 hunks)
- apps/backend/src/modules/status/status.module.ts (1 hunks)
- apps/backend/src/modules/status/status.service.ts (1 hunks)
- apps/backend/src/modules/status/status.types.ts (1 hunks)
🔇 Additional comments (5)
apps/backend/src/modules/status/status.types.ts (1)
1-15: Verify consistent status handling across the codebase.Let's ensure these types are used consistently throughout the application.
✅ Verification successful
Let me generate one more verification script to check for any inconsistent status handling patterns with a corrected regex:
Based on the verification results, I can now generate the final response:
Status types are implemented consistently across the codebase
The verification shows:
- All status assignments use the
SystemStatusenum correctly (OK/ERROR)- Status handling is properly implemented in both the controller and service layers
- No inconsistent string literals are used for status values
- Error states are properly handled using
SystemStatus.ERROR🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for consistent usage of status types and potential status handling patterns # Check for status type usage echo "Checking status type usage patterns..." rg -t ts "SystemStatus\." --context 3 # Check for error handling patterns echo "Checking error handling patterns..." ast-grep --pattern 'catch ($_) { $$$ status: SystemStatus.ERROR $$$ }' # Look for potential inconsistent status handling echo "Checking for potential inconsistent status handling..." rg -t ts "status[\"']?\s*[:=]\s*[\"'](?!ok|error)[^\"']+[\"']" --context 2Length of output: 4662
Script:
#!/bin/bash # Description: Additional verification for status handling patterns # Check for status assignments that might not use SystemStatus enum echo "Checking for potential inconsistent status assignments..." rg -t ts "status\s*[:=]\s*['\"].*['\"]" --context 2 # Check all files that import and use these types echo "Checking files importing status types..." rg -t ts "import.*SystemStatus|import.*StatusObject|import.*SystemStatusService" -l # Check for any try-catch blocks in status-related files echo "Checking error handling in status-related files..." ast-grep --pattern 'try { $$$ } catch ($_) { $$$ }'Length of output: 7216
apps/backend/src/modules/prisma/prisma.service.ts (1)
23-36: Verify the impact on geo functions and departures endpoint.These methods are critical for geo functions which may affect the departures endpoint functionality.
Let's verify the dependencies and usage:
✅ Verification successful
The changes to
getExtensionsandgetExtensionNamesare safe for geo functions and departures endpoint.The code analysis reveals:
- The geo functions (
cubeandearthdistance) are only used inplatform.service.tsfor distance calculations- The status service correctly monitors these extensions via
getExtensionNames()- The departure service doesn't directly use geo functions, it only depends on platform IDs from the database
- The platform service's geo queries are properly protected by the status check
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if these extensions are used in departure-related code echo "Checking for geo function dependencies in departure code..." rg -l "earthdistance|cube" --type ts # Check if there are any existing extension-related error handlers echo "Checking for existing extension error handling..." rg -l "pg_extension.*error|extension.*error" --type tsLength of output: 383
Script:
#!/bin/bash # Let's check the content of status.service.ts to understand the geo function usage echo "Checking status.service.ts content:" cat apps/backend/src/modules/status/status.service.ts # Let's also search for any departure-related endpoints that might use geo functions echo -e "\nChecking for departure endpoints and geo-related functions:" rg -l "departure" --type ts # Search for SQL queries using geo functions echo -e "\nChecking for SQL queries using geo functions:" rg "earth_distance|cube|ll_to_earth" -A 3 --type tsLength of output: 3234
Script:
#!/bin/bash # Let's check the platform service implementation to understand how it's used in departures echo "Checking platform.service.ts content:" cat apps/backend/src/modules/platform/platform.service.ts # Let's also check how departure service uses the platform service echo -e "\nChecking departure.service.ts content:" cat apps/backend/src/modules/departure/departure.service.tsLength of output: 9845
apps/backend/src/modules/status/status.service.ts (1)
1-45: Verify test coverage for StatusService.This service provides critical system health information and should be thoroughly tested. Please ensure there are comprehensive unit tests covering all status check scenarios and error cases.
apps/backend/src/app.module.ts (1)
16-16: LGTM, but this appears to be scope creep.The import follows the project's conventions. However, the addition of
StatusModuleseems outside the PR's stated objectives of extending the departure endpoint functionality.Let's verify if this module is essential for the departure endpoint changes:
apps/backend/src/modules/status/status.controller.ts (1)
79-81: Similar error handling improvements needed.This section has the same error handling issues as the geo functions endpoint.
| export type StatusObject = { | ||
| service: SystemStatusService; | ||
| status: SystemStatus; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding timestamp and message fields.
The status object could be more informative by including a timestamp and an optional message field for error details or additional information.
export type StatusObject = {
service: SystemStatusService;
status: SystemStatus;
+ timestamp: Date;
+ message?: string;
};📝 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.
| export type StatusObject = { | |
| service: SystemStatusService; | |
| status: SystemStatus; | |
| }; | |
| export type StatusObject = { | |
| service: SystemStatusService; | |
| status: SystemStatus; | |
| timestamp: Date; | |
| message?: string; | |
| }; |
| export enum SystemStatus { | ||
| OK = "ok", | ||
| ERROR = "error", | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding more granular status states.
A binary OK/ERROR status might be too simplistic for real-world scenarios. Consider adding intermediate states to better represent system health.
export enum SystemStatus {
OK = "ok",
+ DEGRADED = "degraded",
+ MAINTENANCE = "maintenance",
+ UNKNOWN = "unknown",
ERROR = "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.
| export enum SystemStatus { | |
| OK = "ok", | |
| ERROR = "error", | |
| } | |
| export enum SystemStatus { | |
| OK = "ok", | |
| DEGRADED = "degraded", | |
| MAINTENANCE = "maintenance", | |
| UNKNOWN = "unknown", | |
| ERROR = "error", | |
| } |
| import { StatusController } from "src/modules/status/status.controller"; | ||
| import { StatusService } from "src/modules/status/status.service"; |
There was a problem hiding this comment.
Use relative import paths instead of src-based paths.
Using "src/" in import paths can cause issues with build tools and make the code less maintainable. Consider using relative paths instead.
Apply this diff to fix the imports:
-import { StatusController } from "src/modules/status/status.controller";
-import { StatusService } from "src/modules/status/status.service";
+import { StatusController } from "./status.controller";
+import { StatusService } from "./status.service";📝 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.
| import { StatusController } from "src/modules/status/status.controller"; | |
| import { StatusService } from "src/modules/status/status.service"; | |
| import { StatusController } from "./status.controller"; | |
| import { StatusService } from "./status.service"; |
| async getExtensionNames(): Promise<string[]> { | ||
| const extensions = await this.getExtensions(); | ||
|
|
||
| return extensions.map((ext) => ext.extname); | ||
| } |
There was a problem hiding this comment.
Add null check and error handling for extension mapping.
The mapping operation could fail if the extensions array is null or contains invalid data.
Consider adding defensive programming:
async getExtensionNames(): Promise<string[]> {
const extensions = await this.getExtensions();
-
- return extensions.map((ext) => ext.extname);
+ if (!extensions?.length) {
+ return [];
+ }
+ return extensions.map((ext) => ext.extname).filter(Boolean);
}📝 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.
| async getExtensionNames(): Promise<string[]> { | |
| const extensions = await this.getExtensions(); | |
| return extensions.map((ext) => ext.extname); | |
| } | |
| async getExtensionNames(): Promise<string[]> { | |
| const extensions = await this.getExtensions(); | |
| if (!extensions?.length) { | |
| return []; | |
| } | |
| return extensions.map((ext) => ext.extname).filter(Boolean); | |
| } |
| async getGeoFunctionsStatus(): Promise<StatusObject> { | ||
| const extensionNames = await this.prisma.getExtensionNames(); | ||
| const isOk = | ||
| extensionNames.includes("cube") && | ||
| extensionNames.includes("earthdistance"); | ||
|
|
||
| return { | ||
| service: SystemStatusService.GEO_FUNCTIONS, | ||
| status: isOk ? SystemStatus.OK : SystemStatus.ERROR, | ||
| }; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling and consider caching extension checks.
The extension check is good but could be enhanced with proper error handling and caching to prevent frequent database queries.
private extensionCheckCache: { value: boolean; timestamp: number } | null = null;
private readonly CACHE_DURATION = 5 * 60 * 1000; // 5 minutes
async getGeoFunctionsStatus(): Promise<StatusObject> {
try {
const now = Date.now();
if (this.extensionCheckCache &&
now - this.extensionCheckCache.timestamp < this.CACHE_DURATION) {
return {
service: SystemStatusService.GEO_FUNCTIONS,
status: this.extensionCheckCache.value ? SystemStatus.OK : SystemStatus.ERROR,
};
}
const extensionNames = await this.prisma.getExtensionNames();
const isOk = extensionNames.includes("cube") &&
extensionNames.includes("earthdistance");
this.extensionCheckCache = { value: isOk, timestamp: now };
return {
service: SystemStatusService.GEO_FUNCTIONS,
status: isOk ? SystemStatus.OK : SystemStatus.ERROR,
};
} catch (error) {
return {
service: SystemStatusService.GEO_FUNCTIONS,
status: SystemStatus.ERROR,
error: 'Failed to check database extensions',
};
}
}| async getDbDataStatus(): Promise<StatusObject> { | ||
| const stopCount = await this.prisma.stop.count(); | ||
| const routeCount = await this.prisma.route.count(); | ||
| const platformCount = await this.prisma.platform.count(); | ||
|
|
||
| const isOk = stopCount > 0 && routeCount > 0 && platformCount > 0; | ||
|
|
||
| return { | ||
| service: SystemStatusService.DB_DATA, | ||
| status: isOk ? SystemStatus.OK : SystemStatus.ERROR, | ||
| }; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize database checks and add error handling.
The current implementation makes multiple separate database calls and lacks error handling. Consider using a single query and adding proper error handling.
async getDbDataStatus(): Promise<StatusObject> {
try {
const counts = await this.prisma.$transaction([
this.prisma.stop.count(),
this.prisma.route.count(),
this.prisma.platform.count(),
]);
const [stopCount, routeCount, platformCount] = counts;
const isOk = stopCount > 0 && routeCount > 0 && platformCount > 0;
return {
service: SystemStatusService.DB_DATA,
status: isOk ? SystemStatus.OK : SystemStatus.ERROR,
details: {
stops: stopCount,
routes: routeCount,
platforms: platformCount,
},
};
} catch (error) {
return {
service: SystemStatusService.DB_DATA,
status: SystemStatus.ERROR,
error: 'Failed to check database data',
};
}
}| if (geoFunctionsStatus.status === "error") { | ||
| throw new Error("Geo functions status is not OK"); | ||
| } |
There was a problem hiding this comment.
Improve error handling robustness.
The current error handling uses string literal comparison and throws a generic Error.
Consider using:
- Enum comparison instead of string literal
- A custom exception class
- More detailed error information
-if (geoFunctionsStatus.status === "error") {
- throw new Error("Geo functions status is not OK");
+if (geoFunctionsStatus.status === SystemStatus.ERROR) {
+ throw new ServiceException(
+ `Geo functions check failed: ${geoFunctionsStatus.message || 'Unknown error'}`,
+ 'GEO_FUNCTIONS_ERROR'
+ );Committable suggestion was skipped due to low confidence.
| async getBackendStatus(): Promise<StatusObject[]> { | ||
| return [ | ||
| this.statusService.getBackendStatus(), | ||
| await this.statusService.getGeoFunctionsStatus(), | ||
| await this.statusService.getDbDataStatus(), | ||
| ]; | ||
| } |
There was a problem hiding this comment.
Improve error handling and async operation consistency.
The current implementation has several potential issues:
- No error handling for async operations
- Mixing synchronous and asynchronous calls in the array
- No type checking for the status responses
Consider this safer implementation:
async getBackendStatus(): Promise<StatusObject[]> {
- return [
- this.statusService.getBackendStatus(),
- await this.statusService.getGeoFunctionsStatus(),
- await this.statusService.getDbDataStatus(),
- ];
+ try {
+ const [backendStatus, geoStatus, dbStatus] = await Promise.all([
+ Promise.resolve(this.statusService.getBackendStatus()),
+ this.statusService.getGeoFunctionsStatus(),
+ this.statusService.getDbDataStatus(),
+ ]);
+ return [backendStatus, geoStatus, dbStatus];
+ } catch (error) {
+ // Consider creating a custom exception filter
+ throw new Error(`Failed to fetch system status: ${error.message}`);
+ }
}📝 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.
| async getBackendStatus(): Promise<StatusObject[]> { | |
| return [ | |
| this.statusService.getBackendStatus(), | |
| await this.statusService.getGeoFunctionsStatus(), | |
| await this.statusService.getDbDataStatus(), | |
| ]; | |
| } | |
| async getBackendStatus(): Promise<StatusObject[]> { | |
| try { | |
| const [backendStatus, geoStatus, dbStatus] = await Promise.all([ | |
| Promise.resolve(this.statusService.getBackendStatus()), | |
| this.statusService.getGeoFunctionsStatus(), | |
| this.statusService.getDbDataStatus(), | |
| ]); | |
| return [backendStatus, geoStatus, dbStatus]; | |
| } catch (error) { | |
| // Consider creating a custom exception filter | |
| throw new Error(`Failed to fetch system status: ${error.message}`); | |
| } | |
| } |
| async getPlatformsByDistance(): Promise<StatusObject> { | ||
| const geoFunctionsStatus = | ||
| await this.statusService.getGeoFunctionsStatus(); | ||
|
|
||
| if (geoFunctionsStatus.status === "error") { | ||
| throw new Error("Geo functions status is not OK"); | ||
| } | ||
|
|
||
| return geoFunctionsStatus; | ||
| } |
There was a problem hiding this comment.
Method name doesn't match its functionality.
The method name getPlatformsByDistance suggests functionality related to platform distances, but it actually checks geo functions status.
Rename the method to match its functionality:
-async getPlatformsByDistance(): Promise<StatusObject> {
+async getGeoFunctionsStatus(): Promise<StatusObject> {📝 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.
| async getPlatformsByDistance(): Promise<StatusObject> { | |
| const geoFunctionsStatus = | |
| await this.statusService.getGeoFunctionsStatus(); | |
| if (geoFunctionsStatus.status === "error") { | |
| throw new Error("Geo functions status is not OK"); | |
| } | |
| return geoFunctionsStatus; | |
| } | |
| async getGeoFunctionsStatus(): Promise<StatusObject> { | |
| const geoFunctionsStatus = | |
| await this.statusService.getGeoFunctionsStatus(); | |
| if (geoFunctionsStatus.status === "error") { | |
| throw new Error("Geo functions status is not OK"); | |
| } | |
| return geoFunctionsStatus; | |
| } |
import data from single endpoint
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
apps/backend/src/modules/import/import.service.ts (1)
197-197: Use 'LoggerService' instead of 'console.error' for error logging.To maintain consistent logging throughout the application, use the provided
LoggerServiceinstead ofconsole.error(). This ensures that all logs are handled uniformly and can be properly managed or formatted by the logger.Apply this diff to replace
console.error(error);withthis.logger.error(error);:- console.error(error); + this.logger.error(error);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- apps/backend/src/modules/import/import.service.ts (5 hunks)
- apps/backend/src/modules/import/schema/pid-platforms.schema.ts (0 hunks)
- apps/backend/src/modules/import/schema/pid-stops.schema.ts (1 hunks)
💤 Files with no reviewable changes (1)
- apps/backend/src/modules/import/schema/pid-platforms.schema.ts
🔇 Additional comments (3)
apps/backend/src/modules/import/schema/pid-stops.schema.ts (2)
15-17: LGTM! The new fields align well with the PR objectives.The addition of
gtfsIds,altIdosName, andisMetrofields provides the necessary data structure to support:
- Multiple stop identifiers through
gtfsIdsarray- Metro-only filtering through the optional
isMetroflag
15-24: Verify the impact on existing data imports.The schema changes look comprehensive, but we should verify that existing data imports won't break with these new requirements.
Let's check for any existing data import implementations:
✅ Verification successful
Let me check the import service implementation to understand how the schema is being used.
Let me check if there are any existing data files or test fixtures that use this schema to understand the current data format.
Let me try a different approach to find test files.
Let me check the schema file itself to understand if these are new fields or modifications to existing ones.
Schema changes are safe for existing imports
Based on the git history, this is a new schema file that was just created and modified in the same PR. The changes replace the simple
idfield with more detailed fields (gtfsIds,altIdosName, etc.) to import data from a single endpoint. Since this is a new implementation without any existing data imports depending on it, there's no risk of breaking existing functionality.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for files that might contain PID data import logic rg -l "pidStopsSchema" --type tsLength of output: 141
Script:
#!/bin/bash # Check the implementation of PID stops import rg -A 10 "pidStopsSchema" apps/backend/src/modules/import/import.service.tsLength of output: 721
Script:
#!/bin/bash # Search for JSON/test files that might contain PID stops data rg -l "gtfsIds|altIdosName|isMetro" --type json --type testLength of output: 90
Script:
#!/bin/bash # Search for test files containing PID stops data patterns rg -l "gtfsIds|altIdosName|isMetro" -g "*.test.ts" -g "*.spec.ts" -g "*.test.js" -g "*.spec.js" # Also check for any mock data files fd -e json -e fixture "mock|test|fixture" --exec grep -l "gtfsIds\|altIdosName\|isMetro" {} \;Length of output: 197
Script:
#!/bin/bash # Get the git history of the schema file to see changes git log -p apps/backend/src/modules/import/schema/pid-stops.schema.tsLength of output: 2335
apps/backend/src/modules/import/import.service.ts (1)
114-116: Ensure all 'vehicleType' mappings are valid.When mapping
route.vehicleType.toUpperCase()to theVehicleTypeenum, ensure that all possible values ofroute.vehicleTypecorrespond to valid keys in the enum. Unmatched values will result invehicleTypebeing set tonull, which may not be desired.Consider adding a validation step or a mapping function to handle cases where
route.vehicleTypedoes not match anyVehicleTypekey.
| lines: z | ||
| .object({ | ||
| id: z.coerce.string(), | ||
| name: z.string(), | ||
| type: z.string(), | ||
| }) | ||
| .array(), |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding enum validation for the line type field.
The lines array structure looks good, but the type field could benefit from stricter validation.
Consider defining an enum for valid line types:
const LineType = z.enum(['METRO', 'BUS', 'TRAM']);
// Then use it in the schema:
lines: z
.object({
id: z.coerce.string(),
name: z.string(),
type: LineType,
})
.array(),| vehicleType: | ||
| VehicleType?.[route.vehicleType.toUpperCase()] ?? | ||
| null, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify optional chaining when accessing 'VehicleType'.
Since 'VehicleType' is imported and should always be defined, the optional chaining operator ?. is unnecessary. You can simplify the code by removing it.
Apply this diff to simplify the code:
- vehicleType:
- VehicleType?.[route.vehicleType.toUpperCase()] ??
- null,
+ vehicleType:
+ VehicleType[route.vehicleType.toUpperCase()] ??
+ null,📝 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.
| vehicleType: | |
| VehicleType?.[route.vehicleType.toUpperCase()] ?? | |
| null, | |
| vehicleType: | |
| VehicleType[route.vehicleType.toUpperCase()] ?? | |
| null, |
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Outside diff range and nitpick comments (9)
apps/backend/e2e/utils/generate-test-urls.ts (2)
3-3: Consider a more descriptive type name.While
SearchParamsis functional, a more specific name likeQueryParamTuplesorURLSearchParamPairswould better describe its purpose and structure.-type SearchParams = [searchParam: string, value: string][]; +type URLSearchParamPairs = [paramName: string, paramValue: string][];
1-21: Add unit tests for utility functions.These utility functions are critical for e2e tests, but they themselves should have unit tests to ensure they handle all edge cases correctly.
Would you like me to help create unit tests for these utility functions? I can generate test cases covering:
- Various input types and combinations
- Edge cases and error conditions
- Input validation scenarios
apps/backend/e2e/tests/stop.e2e-spec.ts (1)
Based on the analysis of the test files, I can now provide the final response:
Test coverage needs improvement with error scenarios and edge cases
The current e2e tests only cover happy paths with 200 status codes. The test suite should be enhanced with:
- Error handling tests (400, 404, 500 scenarios)
- Cache behavior verification (CacheModule is imported but not tested)
- Database connection error scenarios
- Invalid parameter tests
- Response time assertions for performance monitoring
Example locations needing additional tests:
apps/backend/e2e/tests/stop.e2e-spec.tsapps/backend/e2e/tests/platform.e2e-spec.tsapps/backend/e2e/tests/departure.e2e-spec.tsapps/backend/e2e/tests/status.e2e-spec.ts🔗 Analysis chain
Line range hint
18-38: Consider adding more test scenarios.The test setup is good, but consider adding these test scenarios:
- Cache behavior testing
- Error handling for database connection issues
- Performance testing with response time assertions
- Edge cases like empty database results
Let's check if there are similar test patterns in other e2e test files:
Let me check the actual test implementations to better understand the test coverage patterns.
Let me check if there are any error handling or cache-related tests in these files.
Let me check for any invalid request tests or error handling scenarios.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for similar test patterns in other e2e test files rg -t ts "it\.(each|describe|test)" "apps/backend/e2e/tests/"Length of output: 557
Script:
#!/bin/bash # Check the test implementations in these files rg -A 10 -B 2 "it\.each" "apps/backend/e2e/tests/"Length of output: 5447
Script:
#!/bin/bash # Search for error handling and cache-related tests rg -B 2 -A 5 "expect.*4|expect.*5|cache|error" "apps/backend/e2e/tests/"Length of output: 7680
Script:
#!/bin/bash # Search for any error status code tests (4xx, 5xx) and invalid request handling rg -B 3 -A 3 "expect\((400|401|403|404|500)|\.expect\((400|401|403|404|500)" "apps/backend/e2e/tests/"Length of output: 102
apps/backend/e2e/tests/status.e2e-spec.ts (2)
17-30: Add error handling for app initialization.While the setup is correct, consider adding error handling for app initialization to make the tests more robust.
beforeAll(async () => { + try { const moduleFixture: TestingModule = await Test.createTestingModule({ imports: [ CacheModule.register(cacheModuleConfig), ConfigModule.forRoot(configModuleConfig), PrismaModule, LoggerModule, StatusModule, ], }).compile(); app = moduleFixture.createNestApplication(); await app.init(); + } catch (error) { + console.error('Failed to initialize test application:', error); + throw error; + } });
1-58: Document test coverage strategy and integrate with overall testing plan.While these tests are a good start, consider:
- Adding JSDoc comments describing the test coverage strategy
- Creating a test plan document outlining which endpoints need e2e coverage
- Integrating these tests with the CI/CD pipeline
This will help track testing progress and ensure comprehensive coverage of the new departure endpoint features mentioned in the PR objectives.
apps/backend/e2e/tests/platform.e2e-spec.ts (2)
Line range hint
40-57: Enhance test assertions for better coverage.While the basic structure is good, consider adding more specific assertions to validate:
- The structure of platform objects in the response
- The filtering behavior when
metroOnlyis true/false- The presence of required platform fields
Example enhancement:
expect(response.body).toBeDefined(); expect(response.body).toBeInstanceOf(Array); expect(response.body.length).toBeGreaterThan(0); +expect(response.body[0]).toHaveProperty('id'); +expect(response.body[0]).toHaveProperty('name'); +if (url.includes('metroOnly=true')) { + expect(response.body.every(platform => platform.isMetro)).toBe(true); +}
Line range hint
98-107: Enhance closest platform response validation.The current assertions don't verify the distance-based ordering or count limits.
Add more specific assertions:
expect(response.body).toBeDefined(); expect(response.body).toBeInstanceOf(Array); expect(response.body.length).toBeGreaterThan(0); +// Verify count parameter is respected +if (url.includes('count=')) { + const count = parseInt(new URL(url, 'http://test').searchParams.get('count')!); + expect(response.body.length).toBeLessThanOrEqual(count); +} +// Verify platforms are ordered by distance +const distances = response.body.map(p => p.distance); +expect([...distances].sort((a, b) => a - b)).toEqual(distances);apps/backend/e2e/tests/departure.e2e-spec.ts (2)
85-102: Consider reducing the number of test stop IDs.The large array of stop IDs might make the tests harder to maintain. Consider:
- Using a smaller representative sample
- Moving these IDs to a separate test constants file
- Adding comments explaining the significance of these specific IDs
40-113: Tests align with PR objectives but need more robustness.While the tests cover all new features (platform IDs, stop IDs, metroOnly filter), consider:
- Adding integration tests with the actual backend service
- Testing rate limiting and error responses
- Adding performance benchmarks for large result sets
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
- apps/backend/e2e/constant/test-label.ts (1 hunks)
- apps/backend/e2e/tests/departure.e2e-spec.ts (3 hunks)
- apps/backend/e2e/tests/import.e2e-spec.ts (1 hunks)
- apps/backend/e2e/tests/platform.e2e-spec.ts (3 hunks)
- apps/backend/e2e/tests/status.e2e-spec.ts (1 hunks)
- apps/backend/e2e/tests/stop.e2e-spec.ts (2 hunks)
- apps/backend/e2e/utils/generate-test-urls.ts (1 hunks)
- apps/backend/src/enums/log.enum.ts (1 hunks)
✅ Files skipped from review due to trivial changes (1)
- apps/backend/e2e/constant/test-label.ts
🔇 Additional comments (8)
apps/backend/src/enums/log.enum.ts (1)
6-6: LGTM! Clean enum addition.The
TESTvalue is well-documented and follows the established naming convention.Let's verify the e2e test implementation status:
✅ Verification successful
E2E tests are implemented and actively using the
TESTenum valueThe verification confirms that:
- The
TESTenum value is already being used inapps/backend/e2e/tests/import.e2e-spec.ts- Multiple e2e test files exist in the codebase under
apps/backend/e2e/tests/- The enum is used correctly with
ImportService.syncStops()method🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for e2e test files that use StopSyncTrigger.TEST # Search for e2e test files using the TEST trigger rg -l "StopSyncTrigger.TEST" --type ts | grep -i "e2e" # Search for potential test files in an e2e directory fd -t f . -E "node_modules" | grep -i "e2e.*test"Length of output: 442
Script:
#!/bin/bash # Let's check the implementation in import.e2e-spec.ts to verify how TEST is used rg -A 5 "StopSyncTrigger.TEST" apps/backend/e2e/tests/import.e2e-spec.tsLength of output: 154
apps/backend/e2e/utils/generate-test-urls.ts (1)
1-1: Verify the import path resolution in e2e tests.The import path starting with "src/" might not resolve correctly in e2e tests as they typically run from a different context.
✅ Verification successful
The results show that importing from "src/" paths is a common pattern across all e2e test files in the project. The
array.utils.tsfile exists atapps/backend/src/utils/array.utils.ts, and there are numerous other imports from the "src/" directory in various e2e test files. This indicates that the project's build/test configuration is properly set up to handle these imports in the e2e testing context.Import path is correctly configured for e2e tests
The import path
"src/utils/array.utils"follows the established pattern used throughout the e2e test suite and resolves to the existing file atapps/backend/src/utils/array.utils.ts. No changes are needed.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if the import path is correct and the file exists # Test: Search for array.utils.ts file fd --type f "array.utils.ts" # Test: Check if there are any other imports from src/ in e2e tests rg --type ts "from ['|\"]src/" apps/backend/e2eLength of output: 2939
apps/backend/e2e/tests/import.e2e-spec.ts (1)
1-35: Verify alignment with PR objectives.This test file focuses on the Import Module, but the PR objectives mention extending the departure endpoint. Consider:
- Adding e2e tests for the new departure endpoint features
- Testing the
metroOnlyfilter parameter- Testing stop ID and platform ID based retrievals
Let's check if departure endpoint tests exist:
Would you like me to help create e2e tests for the new departure endpoint features?
✅ Verification successful
E2e tests already cover the mentioned features
The existing
departure.e2e-spec.tsfile already includes comprehensive test coverage for:
- The departure endpoint features
- The
metroOnlyfilter parameter (both true and false cases)- Stop ID and platform ID based retrievals, including:
- Single platform
- Multiple platforms
- Single stop
- Multiple stops
- Combinations of stops and platforms
- Edge cases with empty arrays
The test file uses a data-driven approach with
generateTestUrlsto systematically test all these scenarios. No additional e2e tests are needed for the features mentioned in the PR objectives.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for departure-related test files fd -e spec.ts -e test.ts departure apps/backend/Length of output: 50
Script:
#!/bin/bash # Let's try a broader search for test files and grep for departure-related content fd -e spec.ts -e test.ts . apps/backend/ | xargs rg -l "departure|Departure" # Also search for the actual endpoint implementation to understand what we're testing rg -l "departure.*endpoint" apps/backend/Length of output: 122
Script:
#!/bin/bash # Let's search for controller files to find the departure endpoint implementation fd -e ts controller apps/backend/src/ # Also search for any test files in the e2e directory fd -e spec.ts -e test.ts . apps/backend/e2e/tests/ # Look for departure-related files in the entire backend rg -i "departure" apps/backend/ -t ts --files-with-matchesLength of output: 799
Script:
#!/bin/bash # Let's examine the existing departure e2e test file cat apps/backend/e2e/tests/departure.e2e-spec.ts # And check the departure controller to see the endpoints cat apps/backend/src/modules/departure/departure.controller.tsLength of output: 6802
apps/backend/e2e/tests/stop.e2e-spec.ts (1)
7-11: LGTM! Well-organized test utilities.The separation of test utilities and constants into dedicated files promotes code reusability and maintainability.
apps/backend/e2e/tests/status.e2e-spec.ts (1)
1-13: LGTM! Well-structured imports and module setup.The imports are properly organized and include all necessary NestJS modules and configurations.
apps/backend/e2e/tests/platform.e2e-spec.ts (1)
7-11: LGTM! Improved test case generation approach.The new utility imports provide a more structured and maintainable way to generate test cases.
apps/backend/e2e/tests/departure.e2e-spec.ts (2)
7-11: LGTM! Well-organized imports.The new utility imports show good separation of concerns between URL generation and test labeling.
40-103: 🛠️ Refactor suggestionConsider adding negative test cases.
While the test coverage for valid scenarios is comprehensive, consider adding tests for:
- Invalid platform/stop IDs
- Malformed parameters
- Empty arrays in platform[] and stop[]
Add these test cases:
const negativeTestUrls = generateTestUrls("/departure", [ [...generateParamsArray("platform[]", "INVALID_ID")], [...generateParamsArray("platform[]", [])], [...generateParamsArray("stop[]", "INVALID_STOP")], [...generateParamsArray("stop[]", [])], [...generateParamsArray("metroOnly", "invalid")] ]);
| export const generateParamsArray = ( | ||
| name: string, | ||
| value: boolean | boolean[] | number | number[] | string | string[] = "", | ||
| ): SearchParams => { | ||
| return toArray(value).map((val) => [name, String(val)]); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve type safety and validation.
The function could benefit from:
- Stricter type definitions
- Proper handling of null/undefined values
- Type-specific default values
+type ValidParamValue = boolean | number | string;
+type ValidParamArray = ValidParamValue[];
+type ParamValueType = ValidParamValue | ValidParamArray;
+
export const generateParamsArray = (
name: string,
- value: boolean | boolean[] | number | number[] | string | string[] = "",
+ value?: ParamValueType | null,
): SearchParams => {
+ if (!name) {
+ throw new Error('Parameter name is required');
+ }
+
+ if (value === null || value === undefined) {
+ return [[name, '']];
+ }
+
return toArray(value).map((val) => [name, String(val)]);
};📝 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.
| export const generateParamsArray = ( | |
| name: string, | |
| value: boolean | boolean[] | number | number[] | string | string[] = "", | |
| ): SearchParams => { | |
| return toArray(value).map((val) => [name, String(val)]); | |
| }; | |
| type ValidParamValue = boolean | number | string; | |
| type ValidParamArray = ValidParamValue[]; | |
| type ParamValueType = ValidParamValue | ValidParamArray; | |
| export const generateParamsArray = ( | |
| name: string, | |
| value?: ParamValueType | null, | |
| ): SearchParams => { | |
| if (!name) { | |
| throw new Error('Parameter name is required'); | |
| } | |
| if (value === null || value === undefined) { | |
| return [[name, '']]; | |
| } | |
| return toArray(value).map((val) => [name, String(val)]); | |
| }; |
| export const generateTestUrls = ( | ||
| path: string, | ||
| urlParams: SearchParams[], | ||
| ): string[] => { | ||
| return urlParams.map((params) => { | ||
| const searchParams = new URLSearchParams(params); | ||
|
|
||
| return `${path}?${searchParams.toString()}`; | ||
| }); | ||
| }; |
There was a problem hiding this comment.
Add input validation and edge cases handling.
The function should validate inputs and handle edge cases:
- Validate that path doesn't already contain query parameters
- Handle empty urlParams array
- Sanitize the path parameter
export const generateTestUrls = (
path: string,
urlParams: SearchParams[],
): string[] => {
+ if (path.includes('?')) {
+ throw new Error('Path should not contain query parameters');
+ }
+
+ const sanitizedPath = path.startsWith('/') ? path : `/${path}`;
+
+ if (!urlParams.length) {
+ return [sanitizedPath];
+ }
+
return urlParams.map((params) => {
const searchParams = new URLSearchParams(params);
- return `${path}?${searchParams.toString()}`;
+ return `${sanitizedPath}?${searchParams.toString()}`;
});
};📝 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.
| export const generateTestUrls = ( | |
| path: string, | |
| urlParams: SearchParams[], | |
| ): string[] => { | |
| return urlParams.map((params) => { | |
| const searchParams = new URLSearchParams(params); | |
| return `${path}?${searchParams.toString()}`; | |
| }); | |
| }; | |
| export const generateTestUrls = ( | |
| path: string, | |
| urlParams: SearchParams[], | |
| ): string[] => { | |
| if (path.includes('?')) { | |
| throw new Error('Path should not contain query parameters'); | |
| } | |
| const sanitizedPath = path.startsWith('/') ? path : `/${path}`; | |
| if (!urlParams.length) { | |
| return [sanitizedPath]; | |
| } | |
| return urlParams.map((params) => { | |
| const searchParams = new URLSearchParams(params); | |
| return `${sanitizedPath}?${searchParams.toString()}`; | |
| }); | |
| }; |
| beforeAll(async () => { | ||
| const moduleFixture: TestingModule = await Test.createTestingModule({ | ||
| imports: [ | ||
| ConfigModule.forRoot(configModuleConfig), | ||
| PrismaModule, | ||
| LoggerModule, | ||
| ], | ||
| providers: [ImportService], | ||
| }).compile(); | ||
|
|
||
| app = moduleFixture.createNestApplication(); | ||
| await app.init(); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding global test setup configuration.
The test module setup could benefit from common configuration that might be needed across multiple e2e test files.
Consider extracting the common setup into a helper:
// apps/backend/e2e/test-utils.ts
export async function createTestingApp() {
const moduleFixture: TestingModule = await Test.createTestingModule({
imports: [
ConfigModule.forRoot(configModuleConfig),
PrismaModule,
LoggerModule,
],
providers: [ImportService],
}).compile();
const app = moduleFixture.createNestApplication();
await app.init();
return app;
}Then simplify the test setup:
- beforeAll(async () => {
- const moduleFixture: TestingModule = await Test.createTestingModule({
- imports: [
- ConfigModule.forRoot(configModuleConfig),
- PrismaModule,
- LoggerModule,
- ],
- providers: [ImportService],
- }).compile();
-
- app = moduleFixture.createNestApplication();
- await app.init();
- });
+ beforeAll(async () => {
+ app = await createTestingApp();
+ });| it("Run import", async () => { | ||
| await app.get(ImportService).syncStops(StopSyncTrigger.TEST); | ||
| }); |
There was a problem hiding this comment.
Enhance test coverage with assertions and cleanup.
The current test executes syncStops but doesn't verify the results. Consider:
- Adding assertions to verify the import was successful
- Adding cleanup in
afterAllto prevent test data persistence - Testing error scenarios
Here's a suggested implementation:
- it("Run import", async () => {
- await app.get(ImportService).syncStops(StopSyncTrigger.TEST);
- });
+ describe('syncStops', () => {
+ let importService: ImportService;
+ let prismaService: PrismaService;
+
+ beforeEach(() => {
+ importService = app.get(ImportService);
+ prismaService = app.get(PrismaService);
+ });
+
+ afterEach(async () => {
+ // Clean up test data
+ await prismaService.stop.deleteMany({
+ where: { syncTrigger: StopSyncTrigger.TEST }
+ });
+ });
+
+ it("should successfully sync stops", async () => {
+ await importService.syncStops(StopSyncTrigger.TEST);
+
+ const stops = await prismaService.stop.findMany({
+ where: { syncTrigger: StopSyncTrigger.TEST }
+ });
+
+ expect(stops.length).toBeGreaterThan(0);
+ stops.forEach(stop => {
+ expect(stop).toHaveProperty('id');
+ expect(stop).toHaveProperty('name');
+ // Add more relevant assertions
+ });
+ });
+
+ it("should handle errors gracefully", async () => {
+ // Mock API failure or other error scenarios
+ await expect(
+ importService.syncStops(StopSyncTrigger.TEST)
+ ).rejects.toThrow();
+ });
+ });Committable suggestion was skipped due to low confidence.
| it.each( | ||
| generateTestUrls("/stop/all", [ | ||
| [], | ||
|
|
||
| [...generateParamsArray("metroOnly")], | ||
|
|
||
| [...generateParamsArray("metroOnly", true)], | ||
|
|
||
| [...generateParamsArray("metroOnly", false)], | ||
| ]), | ||
| )(getRequestTestLabel, async (url) => { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance test coverage and validation.
Good use of parameterized testing to cover different URL combinations. However, consider these improvements:
- Add more specific response schema validation:
expect(response.body[0]).toMatchObject({
id: expect.any(String),
name: expect.any(String),
// add other expected fields
});- Validate that metroOnly filter works correctly:
if (url.includes('metroOnly=true')) {
expect(response.body.every(stop => stop.isMetro)).toBe(true);
}- Add error case tests:
it('should return 400 for invalid metroOnly value', async () => {
await request(app.getHttpServer())
.get('/stop/all?metroOnly=invalid')
.expect(400);
});| it.each(["/status/geo-functions"])(getRequestTestLabel, async (url) => { | ||
| const response = await request(app.getHttpServer()) | ||
| .get(url) | ||
| .expect(200) | ||
| .accept("application/json"); | ||
|
|
||
| expect(response.body).toBeDefined(); | ||
| expect(response.body).toBeInstanceOf(Object); | ||
| expect(response.body.service).toEqual("geo-functions"); | ||
| expect(response.body.status).toEqual("ok"); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add test cases for error scenarios and different status values.
The current test only covers the happy path. Consider adding:
- Test cases for error scenarios
- Validation for different status values
- Constants for the expected values
+const GEO_FUNCTIONS_SERVICE = 'geo-functions';
+const STATUS_VALUES = ['ok', 'error'] as const;
+
+describe('Geo Functions Status', () => {
it.each(["/status/geo-functions"])(getRequestTestLabel, async (url) => {
const response = await request(app.getHttpServer())
.get(url)
.expect(200)
.accept("application/json");
expect(response.body).toBeDefined();
expect(response.body).toBeInstanceOf(Object);
- expect(response.body.service).toEqual("geo-functions");
- expect(response.body.status).toEqual("ok");
+ expect(response.body.service).toEqual(GEO_FUNCTIONS_SERVICE);
+ expect(STATUS_VALUES).toContain(response.body.status);
});
+
+ it('should handle service unavailability', async () => {
+ // Mock service to simulate error
+ // Test error response
+ });
+});Committable suggestion was skipped due to low confidence.
| it.each(["/status"])(getRequestTestLabel, async (url) => { | ||
| const response = await request(app.getHttpServer()) | ||
| .get(url) | ||
| .expect(200) | ||
| .accept("application/json"); | ||
|
|
||
| expect(response.body).toBeDefined(); | ||
| expect(response.body).toBeInstanceOf(Array); | ||
| expect(response.body.length).toEqual(3); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance test assertions for better coverage.
The test could be more robust with the following improvements:
- Define the expected array length as a named constant with documentation.
- Add validation for the structure of each item in the array.
+const EXPECTED_STATUS_SERVICES = 3; // Number of services: database, cache, and geo-functions
+
it.each(["/status"])(getRequestTestLabel, async (url) => {
const response = await request(app.getHttpServer())
.get(url)
.expect(200)
.accept("application/json");
expect(response.body).toBeDefined();
expect(response.body).toBeInstanceOf(Array);
- expect(response.body.length).toEqual(3);
+ expect(response.body.length).toEqual(EXPECTED_STATUS_SERVICES);
+ response.body.forEach(service => {
+ expect(service).toHaveProperty('service');
+ expect(service).toHaveProperty('status');
+ expect(typeof service.service).toBe('string');
+ expect(['ok', 'error']).toContain(service.status);
+ });
});📝 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.
| it.each(["/status"])(getRequestTestLabel, async (url) => { | |
| const response = await request(app.getHttpServer()) | |
| .get(url) | |
| .expect(200) | |
| .accept("application/json"); | |
| expect(response.body).toBeDefined(); | |
| expect(response.body).toBeInstanceOf(Array); | |
| expect(response.body.length).toEqual(3); | |
| }); | |
| const EXPECTED_STATUS_SERVICES = 3; // Number of services: database, cache, and geo-functions | |
| it.each(["/status"])(getRequestTestLabel, async (url) => { | |
| const response = await request(app.getHttpServer()) | |
| .get(url) | |
| .expect(200) | |
| .accept("application/json"); | |
| expect(response.body).toBeDefined(); | |
| expect(response.body).toBeInstanceOf(Array); | |
| expect(response.body.length).toEqual(EXPECTED_STATUS_SERVICES); | |
| response.body.forEach(service => { | |
| expect(service).toHaveProperty('service'); | |
| expect(service).toHaveProperty('status'); | |
| expect(typeof service.service).toBe('string'); | |
| expect(['ok', 'error']).toContain(service.status); | |
| }); | |
| }); |
| const latLonSearchParams = [ | ||
| ...generateParamsArray("latitude", 14.415868050223628), | ||
| ...generateParamsArray("longitude", 50.08187897724985), | ||
| ]; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider making test coordinates more maintainable.
The hardcoded coordinates could make tests brittle. Consider:
- Moving coordinates to a test constants file
- Adding validation for coordinate ranges
- Using multiple test coordinates for edge cases
Example:
// e2e/constant/test-coordinates.ts
export const TEST_COORDINATES = {
PRAGUE_CENTER: {
latitude: 14.415868050223628,
longitude: 50.08187897724985
},
// Add more test locations as needed
};| it.each( | ||
| generateTestUrls("/platform/closest", [ | ||
| [...latLonSearchParams], | ||
|
|
||
| [...latLonSearchParams, ...generateParamsArray("metroOnly")], | ||
|
|
||
| [...latLonSearchParams, ...generateParamsArray("metroOnly", true)], | ||
|
|
||
| [...latLonSearchParams, ...generateParamsArray("metroOnly", false)], | ||
|
|
||
| [ | ||
| ...latLonSearchParams, | ||
| ...generateParamsArray("metroOnly"), | ||
| ...generateParamsArray("count", 10), | ||
| ], | ||
|
|
||
| [ | ||
| ...latLonSearchParams, | ||
| ...generateParamsArray("metroOnly", "true"), | ||
| ...generateParamsArray("count", 10), | ||
| ], | ||
|
|
||
| [ | ||
| ...latLonSearchParams, | ||
| ...generateParamsArray("metroOnly", "false"), | ||
| ...generateParamsArray("count", 10), | ||
| ], | ||
|
|
||
| [ | ||
| ...latLonSearchParams, | ||
| ...generateParamsArray("metroOnly", "true"), | ||
| ...generateParamsArray("count", 200), | ||
| ], | ||
| ]), | ||
| )(getRequestTestLabel, async (url) => { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Validate parameter boundaries and error cases.
While the parameter combinations are comprehensive, consider adding:
- Tests for invalid count values
- Tests for out-of-range coordinates
- Error case handling
Add test cases for error scenarios:
it.each([
'/platform/closest?latitude=invalid&longitude=50.08',
'/platform/closest?latitude=14.41&longitude=invalid',
'/platform/closest?count=0',
'/platform/closest?count=-1',
])(getRequestTestLabel, async (url) => {
await request(app.getHttpServer())
.get(url)
.expect(400)
.accept('application/json');
});| it.each(testUrls)(getRequestTestLabel, async (url) => { | ||
| const response = await request(app.getHttpServer()) | ||
| .get(url) | ||
| .expect(200) | ||
| .accept("application/json"); | ||
|
|
||
| expect(response.body).toBeDefined(); | ||
| expect(response.body).toBeInstanceOf(Array); | ||
| }); |
There was a problem hiding this comment.
Enhance response validation.
The current assertions only verify the response structure at a basic level. Consider adding:
- Validation of departure object properties
- Schema validation using a DTO or interface
- Response headers validation (e.g., cache headers)
- Timeout handling for real API calls
Example enhancement:
interface DepartureResponse {
id: string;
platform: string;
stop: string;
isMetro: boolean;
// add other expected properties
}
it.each(testUrls)(getRequestTestLabel, async (url) => {
const response = await request(app.getHttpServer())
.get(url)
.expect(200)
.accept("application/json")
.timeout(5000); // Add timeout
expect(response.body).toBeDefined();
expect(response.body).toBeInstanceOf(Array);
// Validate each departure
response.body.forEach((departure: DepartureResponse) => {
expect(departure).toMatchObject({
id: expect.any(String),
platform: expect.any(String),
stop: expect.any(String),
isMetro: expect.any(Boolean),
});
});
// Validate headers
expect(response.headers['cache-control']).toBeDefined();
});
krystxf
force-pushed
the
feat/be-stop-departures
branch
from
3cc16b9
to
4e8c977
Compare
krystxf
force-pushed
the
feat/be-stop-departures
branch
from
4e8c977
to
fce6d5a
Compare
krystxf
force-pushed
the
feat/be-stop-departures
branch
from
fce6d5a
to
600f2a6
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
- apps/backend/e2e/jest-e2e.json (1 hunks)
- apps/backend/e2e/tests/departure.e2e-spec.ts (3 hunks)
- apps/backend/e2e/tests/import.e2e-spec.ts (1 hunks)
- apps/backend/e2e/tests/platform.e2e-spec.ts (3 hunks)
- apps/backend/e2e/tests/status.e2e-spec.ts (1 hunks)
- apps/backend/e2e/tests/stop.e2e-spec.ts (2 hunks)
- apps/backend/package.json (1 hunks)
- apps/backend/src/enums/log.enum.ts (1 hunks)
✅ Files skipped from review due to trivial changes (1)
- apps/backend/e2e/jest-e2e.json
🚧 Files skipped from review as they are similar to previous changes (6)
- apps/backend/e2e/tests/import.e2e-spec.ts
- apps/backend/e2e/tests/platform.e2e-spec.ts
- apps/backend/e2e/tests/status.e2e-spec.ts
- apps/backend/e2e/tests/stop.e2e-spec.ts
- apps/backend/package.json
- apps/backend/src/enums/log.enum.ts
🔇 Additional comments (1)
apps/backend/e2e/tests/departure.e2e-spec.ts (1)
105-113: 🛠️ Refactor suggestionAdd timeout handling and enhance response validation.
The previous review comment about response validation is still applicable. Additionally, consider adding timeout handling for the requests:
test.each(testUrls)(getRequestTestLabel, async (url) => { const response = await request(app.getHttpServer()) .get(url) + .timeout(5000) // Add timeout to prevent hanging tests .expect(200) .accept("application/json"); expect(response.body).toBeDefined(); expect(response.body).toBeInstanceOf(Array); + // Add response validation as suggested in the previous review });
| const testUrls = generateTestUrls("/departure", [ | ||
| [...generateParamsArray("platform[]", "U1000Z1P")], | ||
|
|
||
| [...generateParamsArray("platform[]", ["U1000Z1P", "U1000Z1P"])], | ||
|
|
||
| [ | ||
| ...generateParamsArray("platform[]", ["U1000Z1P", "U1000Z1P"]), | ||
| ...generateParamsArray("stop[]"), | ||
| ], | ||
|
|
||
| [...generateParamsArray("stop[]", "U1000")], | ||
|
|
||
| [ | ||
| ...generateParamsArray("stop[]", "U1000"), | ||
| ...generateParamsArray("platform[]"), | ||
| ], | ||
|
|
||
| [ | ||
| ...generateParamsArray("platform[]", ["U1000Z1P", "U1000Z1P"]), | ||
| ...generateParamsArray("stop[]", "U1000"), | ||
| ], | ||
|
|
||
| [ | ||
| ...generateParamsArray("platform[]", ["U1000Z1P", "U1000Z1P"]), | ||
| ...generateParamsArray("stop[]", "U1000"), | ||
| ...generateParamsArray("metroOnly", "true"), | ||
| ], | ||
|
|
||
| [ | ||
| ...generateParamsArray("platform[]", ["U1000Z1P", "U1000Z1P"]), | ||
| ...generateParamsArray("stop[]", "U1000"), | ||
| ...generateParamsArray("metroOnly", false), | ||
| ], | ||
|
|
||
| [ | ||
| ...generateParamsArray("stop[]", ["U1000"]), | ||
| ...generateParamsArray("metroOnly", true), | ||
| ], | ||
|
|
||
| [ | ||
| ...generateParamsArray("stop[]", "U1000"), | ||
| ...generateParamsArray("metroOnly", false), | ||
| ], | ||
|
|
||
| [ | ||
| ...generateParamsArray("stop[]", [ | ||
| "U1", | ||
| "U10", | ||
| "U100", | ||
| "U1000", | ||
| "U1001", | ||
| "U1002", | ||
| "U1004", | ||
| "U1006", | ||
| "U1007", | ||
| "U1008", | ||
| "U1009", | ||
| "U1010", | ||
| "U1011", | ||
| "U1012", | ||
| "U1013", | ||
| ]), | ||
| ], | ||
| ]); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve test case organization and efficiency.
Consider restructuring the test cases for better maintainability and performance:
- Group related test cases using constants
- Consider reducing the number of stops in the large array test case
- Add comments explaining the purpose of each test case group
// Constants for test data
const SINGLE_PLATFORM = ["U1000Z1P"];
const MULTIPLE_PLATFORMS = ["U1000Z1P", "U1000Z1P"];
const SINGLE_STOP = ["U1000"];
const SAMPLE_STOPS = ["U1", "U10", "U100", "U1000"]; // Reduced set for faster execution
// Group 1: Platform-only tests
const platformTests = [
[...generateParamsArray("platform[]", SINGLE_PLATFORM)],
[...generateParamsArray("platform[]", MULTIPLE_PLATFORMS)],
];
// Group 2: Combined platform and stop tests
const combinedTests = [
[
...generateParamsArray("platform[]", MULTIPLE_PLATFORMS),
...generateParamsArray("stop[]", SINGLE_STOP),
],
];
// Group 3: Metro filter tests
const metroFilterTests = [
[
...generateParamsArray("platform[]", MULTIPLE_PLATFORMS),
...generateParamsArray("stop[]", SINGLE_STOP),
...generateParamsArray("metroOnly", true),
],
];
const testUrls = generateTestUrls("/departure", [
...platformTests,
...combinedTests,
...metroFilterTests,
]);
krystxf
force-pushed
the
feat/be-stop-departures
branch
from
600f2a6
to
22c8f64
Compare
krystxf
force-pushed
the
feat/be-stop-departures
branch
from
22c8f64
to
c24bde6
Compare
krystxf
force-pushed
the
feat/be-stop-departures
branch
from
345143f
to
7403d00
Compare
krystxf
force-pushed
the
feat/be-stop-departures
branch
from
7403d00
to
c24bde6
Compare
Changes
metroOnlySummary by CodeRabbit
New Features
metroOnly,platform[], andstop[].StatusControllerwith endpoints to check backend and service statuses.StatusModulefor managing system health checks.PrismaServicewith methods to retrieve database extensions and their names..env.localfile.Bug Fixes
Documentation
getDeparturesfunctionality and its parameters.