Conversation
Also update Jest config
Tests constants and their properties, checking attributes and headers introduced by the @vtex/diagnostics-semconv package
Refactor logger module to remove usage of deprecated attribute
Remove references to deprecated `VTEX_OPERATION_ID` in constants tests
Remove deprecated constant `ATTR_VTEX_OPERATION_ID` from diagnostics-semconv mock
This change disable sorting of object literal keys for both TypeScript and JavaScript files
- Modified the build script to include linting before cleaning and compiling
Upgrade `jest` to version 29.7.0 and `ts-jest` to version 29.4.1
- Modified `jest.config.js` to enable ESM usage in `ts-jest`. - Removed Jest configuration from `package.json` as it is now centralized in `jest.config.js`.
- Added a helper function to retrieve error messages that accommodates different Node.js versions - Updated error message assertions to check for both message prefixes and specific error codes
Modified the promise resolution in the TestServer class to explicitly resolve with undefined instead of no arguments
There was a problem hiding this comment.
Pull Request Overview
This PR fixes CI pipeline issues by updating dependencies, fixing linting errors, and improving test compatibility across Node.js versions.
- Updated Jest and ts-jest to newer versions with proper ESM configuration
- Disabled problematic TSLint object literal sorting rules
- Enhanced error handling in axios tracing tests for better Node.js version compatibility
Reviewed Changes
Copilot reviewed 22 out of 25 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tslint.json | Disabled object-literal-sort-keys rule to fix linting failures |
| package.json | Updated Jest to v29.7.0 and ts-jest to v29.1.0, added linting to build script |
| src/utils/buildFullPath.ts | Fixed semicolon inconsistencies and equality operators |
| src/service/worker/runtime/graphql/schema/schemaDirectives/Auth.ts | Changed loose equality to strict equality operators |
| Multiple telemetry/metrics files | Fixed semicolon inconsistencies and import ordering |
| Test files | Enhanced error handling for Node.js version compatibility |
| Various source files | Fixed trailing commas and semicolon consistency issues |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
|
||
| function parseArgs (authArgs: AuthDirectiveArgs): AuthDirectiveArgs { | ||
| if (authArgs.scope == 'PUBLIC') { | ||
| if (authArgs.scope === 'PUBLIC') { |
There was a problem hiding this comment.
Good improvement changing from loose equality (==) to strict equality (===) operators. This prevents type coercion issues and follows TypeScript best practices.
| field.resolve = async (root, args, ctx, info) => { | ||
| const authArgs = parseArgs(this.args as AuthDirectiveArgs) | ||
| if (!authArgs.scope || authArgs.scope == 'PRIVATE') { | ||
| if (!authArgs.scope || authArgs.scope === 'PRIVATE') { |
There was a problem hiding this comment.
Good improvement changing from loose equality (==) to strict equality (===) operators. This prevents type coercion issues and follows TypeScript best practices.
| const getErrorMessage = (error: AxiosError, expectedPrefix: string): string => { | ||
| // In Node.js 20+, message might be empty, check errors array | ||
| const errorWithErrors = error as any | ||
| if (!error.message && errorWithErrors.errors && errorWithErrors.errors.length > 0) { | ||
| // Find the error that matches our expected prefix | ||
| const matchingError = errorWithErrors.errors.find((err: any) => | ||
| err.message && err.message.startsWith(expectedPrefix) | ||
| ) | ||
| return matchingError ? matchingError.message : error.message |
There was a problem hiding this comment.
[nitpick] The error handling compatibility function is well-implemented, but consider adding proper TypeScript types instead of using 'any'. Define an interface for the expected error structure to improve type safety.
| const getErrorMessage = (error: AxiosError, expectedPrefix: string): string => { | |
| // In Node.js 20+, message might be empty, check errors array | |
| const errorWithErrors = error as any | |
| if (!error.message && errorWithErrors.errors && errorWithErrors.errors.length > 0) { | |
| // Find the error that matches our expected prefix | |
| const matchingError = errorWithErrors.errors.find((err: any) => | |
| err.message && err.message.startsWith(expectedPrefix) | |
| ) | |
| return matchingError ? matchingError.message : error.message | |
| interface ErrorWithErrorsArray extends AxiosError { | |
| errors?: { message?: string }[] | |
| } | |
| const getErrorMessage = (error: AxiosError, expectedPrefix: string): string => { | |
| // In Node.js 20+, message might be empty, check errors array | |
| const errorWithErrors = error as ErrorWithErrorsArray | |
| if (!error.message && errorWithErrors.errors && errorWithErrors.errors.length > 0) { | |
| // Find the error that matches our expected prefix | |
| const matchingError = errorWithErrors.errors.find( | |
| (err) => err.message && err.message.startsWith(expectedPrefix) | |
| ) | |
| return matchingError ? matchingError.message! : error.message |
|
|
||
| // Check if error message starts with prefix OR if it's empty but we have the expected error code | ||
| const hasExpectedPrefix = errorMessage && errorMessage.startsWith(expectedPrefix) | ||
| const hasExpectedCode = !errorMessage && (error as AxiosError).code === 'ECONNREFUSED' |
There was a problem hiding this comment.
[nitpick] The hardcoded 'ECONNREFUSED' error code should be extracted to a constant or made configurable to improve maintainability and make the test expectations more explicit.
What is the purpose of this pull request?
This PR aims to fix CI pipeline issues where lint and test jobs were failing. The changes include:
What problem is this solving?
The CI pipeline was experiencing consistent failures in both lint and test jobs, preventing successful builds and deployments. The main issues were:
How should this be manually tested?
Screenshots or example usage
Types of changes