EPMRPP-89496 || Migrate client-javascript to TypeScript #229
Conversation
| const { OUTPUT_TYPES } = require('../lib/constants/outputs'); | ||
| const RPClient = require('../build/lib/report-portal-client').default; | ||
| const helpers = require('../build/lib/helpers'); | ||
| const { OUTPUT_TYPES } = require('../build/lib/constants/outputs'); |
There was a problem hiding this comment.
I suppose the tests should test the sources, not built version.
So, please revert the imports.
Also test files should be also migrated to TypeScript.
package.json
Outdated
| "eslint-plugin-node": "^11.1.0", | ||
| "eslint-plugin-prettier": "^4.2.1", | ||
| "jest": "^29.7.0", | ||
| "jest": "^28.1.3", |
There was a problem hiding this comment.
Let's use the latest jest as it is a dev dependency it won't affect the users work with older node versions.
WalkthroughConverted the codebase from CommonJS JavaScript to TypeScript ES modules across core libraries, tests, and tooling; introduced typed APIs and types file, migrated/restyled constants, helpers, client, rest client, logger, statistics, and packaging, added/updated tests, and adjusted ESLint/Jest/TS configs and package exports. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant TestRunner
participant API as PublicReportingAPI
participant Proc as process
TestRunner->>API: PublicReportingAPI.setDescription(text, suite?)
API->>Proc: process.emit(EVENTS.SET_DESCRIPTION, {text, suite})
TestRunner->>API: PublicReportingAPI.addLog(log, suite?)
API->>Proc: process.emit(EVENTS.ADD_LOG, {log, suite})
TestRunner->>API: PublicReportingAPI.setStatus(status, suite?)
API->>Proc: process.emit(EVENTS.SET_STATUS, {status, suite})
note right of API: Thin TS wrapper emitting events
sequenceDiagram
autonumber
participant Caller
participant RP as RPClient
participant Rest as RestClient
participant Axios as AxiosInstance
participant Logger as addLogger
Caller->>RP: new RPClient(options, agentParams)
RP->>Rest: new RestClient(restOptions)
alt Rest debug enabled
Rest->>Logger: addLogger(Axios)
end
RP->>Rest: POST /launch (startLaunch)
Rest-->>RP: { id, uuid }
RP->>Rest: POST /item (startTestItem with {name, codeRef})
Rest-->>RP: { id, uuid }
RP->>Rest: PUT /item/{id} (finishTestItem)
Rest-->>RP: ack
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Pre-merge checks (1 passed, 2 warnings)❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
Poem
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. ✨ Finishing Touches🧪 Generate unit tests
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: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
__tests__/client-id.spec.ts (1)
25-25: Harden tests: ignore ENOENT on unlink and ensure ~/.rp existsDirect
unlinkfails if the file doesn’t exist; writing may fail if~/.rpwasn’t created yet. This can flake on clean CI or when tests run in isolation.Apply this diff:
const unlink = util.promisify(fs.unlink); const readFile = util.promisify(fs.readFile); const writeFile = util.promisify(fs.writeFile); +const safeUnlink = async (filePath: string) => { + try { + await unlink(filePath); + } catch (e) { + if ((e as NodeJS.ErrnoException).code !== 'ENOENT') throw e; + } +}; + +const ensureDir = async (filePath: string) => { + await fs.promises.mkdir(path.dirname(filePath), { recursive: true }); +}; @@ - await unlink(clientIdFile); + await safeUnlink(clientIdFile); @@ - await unlink(clientIdFile); + await safeUnlink(clientIdFile); @@ - await unlink(clientIdFile); + await safeUnlink(clientIdFile); @@ - await unlink(clientIdFile); - const clientId = uuidv4(); - await writeFile(clientIdFile, `client.id=${clientId}\n`, 'utf-8'); + await safeUnlink(clientIdFile); + const clientId = uuidv4(); + await ensureDir(clientIdFile); + await writeFile(clientIdFile, `client.id=${clientId}\n`, 'utf-8'); @@ - await unlink(clientIdFile); - const clientId = uuidv4(); - await writeFile(clientIdFile, `client.id=${clientId}\ntest.property=555\n`, 'utf-8'); + await safeUnlink(clientIdFile); + const clientId = uuidv4(); + await ensureDir(clientIdFile); + await writeFile(clientIdFile, `client.id=${clientId}\ntest.property=555\n`, 'utf-8'); @@ - await unlink(clientIdFile); - const clientId = uuidv4(); - await writeFile(clientIdFile, `test.property=555\nclient.id=${clientId}\n`, 'utf-8'); + await safeUnlink(clientIdFile); + const clientId = uuidv4(); + await ensureDir(clientIdFile); + await writeFile(clientIdFile, `test.property=555\nclient.id=${clientId}\n`, 'utf-8'); @@ - await unlink(clientIdFile); + await safeUnlink(clientIdFile); await writeFile(clientIdFile, `test.property=555`, 'utf-8');Also applies to: 36-36, 43-43, 53-54, 64-65, 72-72
statistics/client-id.ts (1)
21-31: Narrow types and safely parse INI contentAvoid any; store strings and coerce parsed values to string.
Apply:
-async function storeClientId(clientId: string) { - const properties: Record<string, any> = {}; +async function storeClientId(clientId: string) { + const properties: Record<string, string> = {}; if (await exists(RP_PROPERTIES_FILE_PATH)) { const propertiesContent = await readFile(RP_PROPERTIES_FILE_PATH, ENCODING); - Object.assign(properties, ini.parse(propertiesContent)); + const parsed = ini.parse(propertiesContent) as Record<string, unknown>; + for (const [k, v] of Object.entries(parsed)) { + properties[k] = typeof v === 'string' ? v : String(v); + } } properties[CLIENT_ID_KEY] = clientId; const propertiesContent = ini.stringify(properties); await mkdir(RP_FOLDER_PATH, { recursive: true }); await writeFile(RP_PROPERTIES_FILE_PATH, propertiesContent, ENCODING); }__tests__/report-portal-client.spec.ts (1)
51-62: Inconsistent constructor usage in tests.The test on line 52 uses the old single-argument constructor form while the rest of the tests use the new two-argument form with agent parameters. This should be updated for consistency.
Apply this diff to fix the inconsistency:
- const client = new RPClient({ - apiKey: 'test', - project: 'test', - endpoint: 'https://abc.com', - debug: true, - }); + const client = new RPClient( + { + apiKey: 'test', + project: 'test', + endpoint: 'https://abc.com', + debug: true, + }, + { name: 'test-agent', version: '1.0.0' }, + );lib/rest.ts (1)
9-13: axios-retry is attached to the wrong axios instance. No retries currently applied.You create a custom axios instance via axios.create; the top-level axiosRetry(axios, ...) won't affect it. Attach retries to this.axiosInstance and remove the global call.
-axiosRetry(axios, { - retryDelay: () => 100, - retries: 10, - retryCondition: axiosRetry.isRetryableError, -});this.axiosInstance = axios.create({ timeout: DEFAULT_MAX_CONNECTION_TIME_MS, headers: this.headers, ...this.getRestConfig(), }); + + axiosRetry(this.axiosInstance, { + retryDelay: () => 100, + retries: 10, + retryCondition: axiosRetry.isRetryableError, + });Also applies to: 41-50
🧹 Nitpick comments (47)
.eslintrc (1)
7-11: Avoid redundant 'plugin:import/recommended' extend'airbnb-typescript/base' already brings import rules; keeping both is redundant and can cause duplicate/conflicting messages.
Apply this diff:
"extends": [ "airbnb-typescript/base", "plugin:@typescript-eslint/recommended", "plugin:prettier/recommended", - "plugin:import/recommended" ],__tests__/publicReportingAPI.spec.ts (1)
4-5: Restore spies after each testPrevents cross-test leakage of
process.emitspies.Apply this diff:
describe('PublicReportingAPI', () => { + afterEach(() => { + jest.restoreAllMocks(); + });__tests__/client-id.spec.ts (1)
1-6: Optional: isolate from real HOMEConsider stubbing
os.homedir()to a temp dir to avoid touching user files and improve test isolation.__tests__/statistics.spec.ts (2)
51-51: Avoidanyin mocksType the mocked Axios response to satisfy lint without suppressions.
Apply this diff:
-import axios from 'axios'; +import axios, { AxiosResponse } from 'axios'; @@ - jest.spyOn(axios, 'post').mockResolvedValue({} as any); + jest.spyOn(axios, 'post').mockResolvedValue({} as unknown as AxiosResponse); @@ - jest.spyOn(axios, 'post').mockResolvedValue({} as any); + jest.spyOn(axios, 'post').mockResolvedValue({} as unknown as AxiosResponse);Also applies to: 69-69
16-16: Relax Node version regex
interpretermay be single- or double-digit major; use\d+to avoid brittleness.Apply this diff:
- interpreter: expect.stringMatching(/Node\.js \d{2}\.\d+\.\d+/), + interpreter: expect.stringMatching(/Node\.js \d+\.\d+\.\d+/),__tests__/config.spec.ts (1)
29-29: Replace explicitanywithunknownto satisfy lintKeeps runtime the same while avoiding
no-explicit-any.Apply this diff:
- getRequiredOption({ other: 1 } as any, 'project'); + getRequiredOption({ other: 1 } as unknown as Record<string, unknown>, 'project'); @@ - getClientConfig('options' as any); + getClientConfig('options' as unknown as Record<string, unknown>);Also applies to: 76-76
__tests__/outputs.spec.ts (1)
1-1: Strengthen FILE handler assertionVerify it references the expected helper.
Apply this diff:
-import { OUTPUT_TYPES } from '../lib/constants/outputs'; +import { OUTPUT_TYPES } from '../lib/constants/outputs'; +import * as helpers from '../lib/helpers'; @@ describe('FILE', () => { it('should be a function reference to helpers.saveLaunchUuidToFile', () => { - expect(typeof OUTPUT_TYPES.FILE).toBe('function'); - expect(OUTPUT_TYPES.FILE).toBeDefined(); + expect(typeof OUTPUT_TYPES.FILE).toBe('function'); + expect(OUTPUT_TYPES.FILE).toBe(helpers.saveLaunchUuidToFile); }); });Also applies to: 47-51
lib/constants/outputs.ts (1)
3-8: Add an explicit, immutable type for OUTPUT_TYPESPrevents accidental mutation and locks API shapes.
Apply this diff:
-export const OUTPUT_TYPES = { +type OutputTypes = Readonly<{ + STDOUT: (launchUuid: string) => void; + STDERR: (launchUuid: string) => void; + ENVIRONMENT: (launchUuid: string) => string; + FILE: typeof helpers.saveLaunchUuidToFile; +}>; + +export const OUTPUT_TYPES: OutputTypes = { STDOUT: (launchUuid: string) => console.log(`Report Portal Launch UUID: ${launchUuid}`), STDERR: (launchUuid: string) => console.error(`Report Portal Launch UUID: ${launchUuid}`), ENVIRONMENT: (launchUuid: string) => (process.env.RP_LAUNCH_UUID = launchUuid), FILE: helpers.saveLaunchUuidToFile, };lib/logger.ts (2)
4-5: Make interceptor registration idempotentAvoid duplicate logs and interceptor leaks if
addLoggeris called multiple times on the same instance.Apply this diff:
export const addLogger = (axiosInstance: AxiosInstance): void => { + const LOGGER_ATTACHED = Symbol.for('rp.addLogger.attached'); + if ((axiosInstance as any)[LOGGER_ATTACHED]) return; + (axiosInstance as any)[LOGGER_ATTACHED] = true;
2-2: Type-safe startTime and use console.error on failuresAvoid
(any)casts; add a typedstartTimeand log errors viaconsole.error.Apply this diff:
-import { AxiosInstance, AxiosResponse, AxiosError, InternalAxiosRequestConfig } from 'axios'; +import { AxiosInstance, AxiosResponse, AxiosError, InternalAxiosRequestConfig } from 'axios'; + +type TimedConfig = InternalAxiosRequestConfig & { startTime?: number }; @@ - axiosInstance.interceptors.request.use((config: InternalAxiosRequestConfig) => { + axiosInstance.interceptors.request.use((config: InternalAxiosRequestConfig) => { const startDate = new Date(); - (config as any).startTime = startDate.valueOf(); + (config as TimedConfig).startTime = startDate.valueOf(); @@ - axiosInstance.interceptors.response.use( - (response: AxiosResponse) => { + axiosInstance.interceptors.response.use( + (response: AxiosResponse) => { const date = new Date(); - const { status, config } = response as AxiosResponse & { config: InternalAxiosRequestConfig }; + const { status, config } = response; console.log( - `Response status=${status} url=${config.url} time=${ - date.valueOf() - ((config as any).startTime ?? 0) + `Response status=${status} url=${config.url} time=${ + date.valueOf() - ((config as TimedConfig).startTime ?? 0) }ms [${date.toISOString()}]`, ); return response; }, (error: AxiosError) => { const date = new Date(); - const { response, config } = error as AxiosError & { config: InternalAxiosRequestConfig }; + const { response, config } = error; const status = response ? response.status : null; - console.log( + console.error( `Response ${status ? `status=${status}` : `message='${error.message}'`} url=${ - config && config.url + config && (config as TimedConfig).url } time=${ - date.valueOf() - ((config && (config as any).startTime) ?? 0) + date.valueOf() - ((config && (config as TimedConfig).startTime) ?? 0) }ms [${date.toISOString()}]`, ); return Promise.reject(error); }, );Also applies to: 5-12, 15-26, 27-41
statistics/client-id.ts (1)
12-19: Add explicit return types for stronger API contractsThese functions always resolve to string/null and string respectively.
Apply:
-async function readClientId() { +async function readClientId(): Promise<string | null> { if (await exists(RP_PROPERTIES_FILE_PATH)) { const propertiesContent = await readFile(RP_PROPERTIES_FILE_PATH, ENCODING); const properties = ini.parse(propertiesContent); return properties[CLIENT_ID_KEY]; } return null; } -export async function getClientId() { +export async function getClientId(): Promise<string> { let clientId = await readClientId(); if (!clientId) { clientId = uuidv4(); try { await storeClientId(clientId); } catch (ignore) { // do nothing on saving error, client ID will be always new } } return clientId; }Also applies to: 33-44
__tests__/rest.spec.ts (2)
2-3: Remove unused imports/vars to silence test warningsClean up unused symbols flagged by CI.
Apply:
-import isEqual from 'lodash/isEqual'; -import http from 'http'; +// (removed unused imports) - const unauthorizedErrorMessage = - 'Request failed with status code 403: ' + - '{"error":"unauthorized","error_description":"Full authentication is required to access this resource"}'; +// (removed unused constant) - const client = new RestClient(optionsWithLoggerEnabled); + new RestClient(optionsWithLoggerEnabled);Also applies to: 27-29, 46-46
23-26: Fix typo in variable name: unathorizedError → unauthorizedErrorImproves readability and consistency.
Apply:
- const unathorizedError = { + const unauthorizedError = { error: 'unauthorized', error_description: 'Full authentication is required to access this resource', }; ... - const scope = nock(options.baseURL).get('/users').reply(403, unathorizedError); + const scope = nock(options.baseURL).get('/users').reply(403, unauthorizedError);Also applies to: 109-109
__tests__/helpers.spec.ts (3)
25-25: Typo: “miliseconds” → “milliseconds”Minor comment fix.
Apply:
- expect(new Date().getTime() - helpers.now()).toBeLessThan(100); // less than 100 miliseconds difference + expect(new Date().getTime() - helpers.now()).toBeLessThan(100); // less than 100 milliseconds difference
48-55: Model fs.open failures via callback, not throwfs.open is callback-based; throw in mock is not representative. Invoke the callback with an error.
Apply:
- const mockError = new Error('File system error'); - jest.spyOn(fs, 'open').mockImplementation(() => { - throw mockError; - }); + const mockError = new Error('File system error'); + jest.spyOn(fs, 'open').mockImplementation(((...args: any[]) => { + const cb = args[args.length - 1] as Function; + cb(mockError); + return undefined as any; + }) as any);
148-155: Same as above for saveLaunchUuidToFile: use callback to return errorAligns with Node’s fs.open contract.
Apply:
- const mockError = new Error('File system error'); - jest.spyOn(fs, 'open').mockImplementation(() => { - throw mockError; - }); + const mockError = new Error('File system error'); + jest.spyOn(fs, 'open').mockImplementation(((...args: any[]) => { + const cb = args[args.length - 1] as Function; + cb(mockError); + return undefined as any; + }) as any);__tests__/report-portal-client.spec.ts (2)
1-1: Consider removing @ts-nocheck in favor of targeted suppressions.Using
@ts-nocheckdisables all TypeScript checking for the entire file. Since this is a test file that needs to access private properties for testing purposes, consider using more targeted@ts-ignorecomments only where needed, which you're already doing in specific places. This will maintain better type safety throughout the rest of the test file.
475-476: Mock function types may need adjustment.The mock implementations use
jest.fn().mockResolvedValue()which might not align with the actual TypeScript types. Consider verifying that these mock types match the expected function signatures.Consider adding type assertions to ensure mock compatibility:
- resolveFinish: jest.fn().mockResolvedValue(undefined), + resolveFinish: jest.fn().mockResolvedValue(undefined) as jest.Mock<Promise<void>>,Also applies to: 478-479, 760-761
lib/commons/config.ts (2)
45-49: Consider validating launchUuidPrintOutput value.The code assumes that the uppercased value will be a valid key of
OUTPUT_TYPES. If an invalid value is provided, it will silently fall back toSTDOUT. Consider adding validation or logging when an invalid output type is specified.const launchUuidPrintOutputType = ( getOption(options, 'launchUuidPrintOutput', 'STDOUT') ?? 'STDOUT' ) .toString() .toUpperCase() as OutputTypeKey; + + if (!(launchUuidPrintOutputType in OUTPUT_TYPES)) { + console.warn(`Invalid launchUuidPrintOutput value: ${options.launchUuidPrintOutput}. Falling back to STDOUT.`); + }
57-60: Type assertions may hide runtime issues.The type assertions (
as string) on lines 57-60 could potentially hide runtime type mismatches. SincegetRequiredOptionshould already ensure these values exist, consider whether these assertions are necessary.Since
getRequiredOptionandgetApiKeyalready ensure non-null values, you might be able to rely on TypeScript's type inference instead of explicit assertions:calculatedOptions = { - apiKey: apiKey as string, - project: project as string, - endpoint: endpoint as string, - launch: options.launch as string, + apiKey, + project, + endpoint, + launch: options.launch || '',statistics/statistics.ts (1)
10-12: Consider replacing custom hasOption with native methods.The
hasOptionhelper function essentially duplicatesObject.prototype.hasOwnProperty.call. Since you're already using this pattern elsewhere in the codebase, consider using it directly or leveraging optional chaining which is more idiomatic in modern TypeScript.-const hasOption = (options: any, optionName: string): boolean => { - return Object.prototype.hasOwnProperty.call(options, optionName); -}; // ... in getEventParams method: - if (agentParams && hasOption(agentParams, 'name') && agentParams.name) { + if (agentParams?.name) { params.agent_name = agentParams.name; } - if (agentParams && hasOption(agentParams, 'version') && agentParams.version) { + if (agentParams?.version) { params.agent_version = agentParams.version; }lib/publicReportingAPI.ts (3)
1-6: Deduplicate Attribute type; import the shared interface.Avoid drifting types by reusing lib/types.ts. Also mark it as a type-only import.
-import { EVENTS } from './constants/events'; +import { EVENTS } from './constants/events'; +import type { Attribute } from './types'; - -type Attribute = { - key?: string; - value: string; -};
27-27: Remove unnecessary any-casts on process.emit.EVENTS values are strings; payloads are plain objects. The casts add no value and hide type issues.
- process.emit(EVENTS.SET_DESCRIPTION as any, { text, suite } as any); + process.emit(EVENTS.SET_DESCRIPTION, { text, suite }); - process.emit(EVENTS.ADD_ATTRIBUTES as any, { attributes, suite } as any); + process.emit(EVENTS.ADD_ATTRIBUTES, { attributes, suite }); - process.emit(EVENTS.ADD_LOG as any, { log, suite } as any); + process.emit(EVENTS.ADD_LOG, { log, suite }); - process.emit(EVENTS.ADD_LAUNCH_LOG as any, log as any); + process.emit(EVENTS.ADD_LAUNCH_LOG, log); - process.emit(EVENTS.SET_TEST_CASE_ID as any, { testCaseId, suite } as any); + process.emit(EVENTS.SET_TEST_CASE_ID, { testCaseId, suite }); - process.emit(EVENTS.SET_LAUNCH_STATUS as any, status as any); + process.emit(EVENTS.SET_LAUNCH_STATUS, status); - process.emit(EVENTS.SET_STATUS as any, { status, suite } as any); + process.emit(EVENTS.SET_STATUS, { status, suite });Also applies to: 36-36, 45-45, 53-53, 62-62, 70-70, 79-79
20-21: Consider a single export style to avoid API ambiguity.Either keep both named and default exports intentionally, or switch to
export default class ...to enforce one import style. Confirm test/import usage before changing.Also applies to: 83-83
lib/helpers.ts (5)
37-42: Tighten typing for readLaunchesFromFile; drop redundant cast and nullish coalescing.Use a type guard to filter undefined.
- const validIds = ids.filter((id: string | undefined) => id !== undefined) ?? []; - return validIds as Array<string>; + const validIds = ids.filter((id): id is string => id !== undefined); + return validIds;
17-20: Minor: prefer repeat() over Array(...).join for clarity.- return (len < MIN ? name + new Array(MIN - len + 1).join('.') : name).slice(-MAX); + return (len < MIN ? name + '.'.repeat(MIN - len) : name).slice(-MAX);
91-97: Avoid trailing empty brackets when params reduce to an empty list.Return codeRef without [] if no values remain.
- return `${codeRef}[${parameters}]`; + return parameters.length ? `${codeRef}[${parameters.join(',')}]` : codeRef;
13-15: Regex robustness for extracting UUIDs (optional).If glob returns full paths, make the pattern path-safe.
- return filename.match(/rplaunch-(.*)\.tmp/)?.[1]; + return filename.match(/rplaunch-([^/]+)\.tmp$/)?.[1];Also applies to: 36-41
26-34: Mark getServerResult as deprecated at the type level.Surface intent to consumers and IDEs.
-// TODO: deprecate and remove +/** + * @deprecated Use RestClient directly and its typed request methods. + */lib/rest.ts (5)
96-104: Exclude debug from axios config payload.getRestConfig should avoid passing non-axios keys like debug.
- if (key !== 'agent') { + if (key !== 'agent' && key !== 'debug') { acc[key] = this.restClientConfig![key]; }
72-75: Header merge and Host casing.
- Preserve constructor-level headers when adding request headers.
- Use canonical Host casing or omit entirely (Node sets it).
- headers: { - HOST: new URL(url).host, - ...(options.headers || {}), - }, + headers: { + ...(this.headers || {}), + Host: new URL(url).host, + ...(options.headers || {}), + },Alternatively, drop the Host line unless a specific upstream requires it.
124-132: Avoid sending a body on GET requests.Pass undefined data for GET to prevent odd proxies from rejecting it.
- return this.request( - 'GET', - this.buildPath(path), - {}, - { + return this.request('GET', this.buildPath(path), undefined, { ...options, - }, - ); + });- return this.request( - 'GET', - this.buildPathToSyncAPI(path), - {}, - { + return this.request('GET', this.buildPathToSyncAPI(path), undefined, { ...options, - }, - ); + });Also applies to: 147-156
52-58: Path building resilience (optional).Using URL to compose paths avoids accidental double slashes and handles base URLs with or without trailing slash.
Example:
private buildPath(path: string): string { return new URL(path, this.baseURL.endsWith('/') ? this.baseURL : `${this.baseURL}/`).toString(); }
1-26: Consider making retry/timeout tunable via RestClientOptions.Expose retries, retryDelay, and timeout to callers through restClientConfig to avoid hard-coded behaviors.
lib/types.ts (4)
23-26: Narrow loosely-typed string fields to explicit unionsStricter unions improve DX and prevent typos in public API.
Apply:
export interface ClientConfig { @@ - mode?: string; + mode?: 'DEFAULT' | 'DEBUG'; @@ export type SaveLogRQ = { - level?: string; + level?: 'TRACE' | 'DEBUG' | 'INFO' | 'WARN' | 'ERROR' | 'FATAL'; message?: string; time?: number; };Also applies to: 90-92
105-105: Fix MapType to reflect undefined lookupsIndexing a plain object can return undefined; current type masks that and weakens safety in RPClient.
Apply:
-export type MapType = { [tempId: string]: ItemObj }; +export type MapType = Record<string, ItemObj | undefined>;This aligns with runtime guards like “if (!launchObj) …” in RPClient.
18-19: Consider narrowing launchUuidPrintOutputUnion with string forces ts-ignore downstream. Prefer function-only API or a tagged union to represent destinations (e.g., 'STDOUT' | ((uuid: string) => void)).
21-22: Remove stray comment markerThe standalone “//” is a formatting artifact.
Apply:
- token?: string; - // + token?: string;lib/report-portal-client.ts (5)
646-653: Avoid mutating request objects with Object.assign + boolean short-circuitUsing
Object.assign(saveLogRQ, {...}, isItemUuid && { itemUuid })mutates inputs and relies on coercion offalse. Prefer immutable spreads.Apply:
- return this.restClient.create( - url, - Object.assign(saveLogRQ, { launchUuid }, isItemUuid && { itemUuid }), - ); + return this.restClient.create(url, { + ...saveLogRQ, + launchUuid, + ...(isItemUuid ? { itemUuid } : {}), + });- return this.getRequestLogWithFile( - Object.assign(saveLogRQ, { launchUuid }, isItemUuid && { itemUuid }), - fileObj, - ); + return this.getRequestLogWithFile( + { ...saveLogRQ, launchUuid, ...(isItemUuid ? { itemUuid } : {}) }, + fileObj, + );Also applies to: 673-680
469-477: Don’t mutate typed payload with “as any”; compose the request object onceCasting to any to add
launchUuidhides type issues.Apply:
- (testItemData as any).launchUuid = realLaunchId; this.logDebug(`Start test item with tempId ${tempId}`, testItemData); - this.restClient.create(url, testItemData).then( + this.restClient.create(url, { ...testItemData, launchUuid: realLaunchId }).then(Optionally extend
TestItemDataRQwith an internal union iflaunchUuidis intentionally internal-only.
3-3: Avoid importing URLSearchParams from 'url' to improve bundler compatibilityIt’s globally available in recent Node and browsers; importing from 'url' can break browser builds.
Apply:
-import { URLSearchParams } from 'url'; +// Use global URLSearchParams
33-35: Map indexing can be undefined; current typing hides thatSeveral guards check for missing entries (e.g.,
if (!launchObj)), butMapTypecurrently suggests all keys exist. After changingMapTypeas suggested, these checks become type-safe.No code changes needed here if you adopt the
Record<string, ItemObj | undefined>fix in lib/types.ts.Also applies to: 51-52, 142-146, 368-376, 638-645
711-743: Multipart JSON part includes an extra blank lineThere are three EOLs before
JSON.stringify(jsonPart). Two are sufficient per spec; the extra blank line is harmless but can be trimmed.Apply:
- eol + - eol + - eol + + eol + + eol +tsconfig.json (4)
10-11: Declaration maps disabled — confirm intent or enable for consumersTurning off declaration maps makes debugging and “go to definition” in downstream projects harder. If size is a concern, keep them disabled for publish but enable in a separate build config; otherwise consider re-enabling.
Apply to re-enable here:
-// "declarationMap": true, + "declarationMap": true,
23-26: Include vs exclude for tests is contradictoryYou both include and exclude "tests". Exclude wins, so including them is redundant and confusing. Suggest removing tests from "include" for clarity.
- "include": [ - "statistics/**/*", "lib/**/*", "__tests__/**/*"], + "include": ["statistics/**/*", "lib/**/*"],
13-14: Module format: confirm CommonJS is desired after ESM migrationWith "module": "commonjs", emitted code is CJS. If the repo is moving to ESM, consider "module": "ES2020" (or "NodeNext") and possibly a dual-build (CJS + ESM) to avoid breaking consumers.
12-12: Minor: redundant lib entry"es2016" already covers Array.prototype.includes; "es2016.array.include" is unnecessary. You can simplify:
- "lib": ["es2016", "es2016.array.include"], + "lib": ["es2016"],
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
lib/.DS_Storeis excluded by!**/.DS_Storepackage-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (32)
.eslintrc(1 hunks)__tests__/client-id.spec.ts(4 hunks)__tests__/config.spec.ts(3 hunks)__tests__/helpers.spec.ts(7 hunks)__tests__/outputs.spec.ts(1 hunks)__tests__/publicReportingAPI.spec.ts(1 hunks)__tests__/report-portal-client.spec.ts(10 hunks)__tests__/rest.spec.js(0 hunks)__tests__/rest.spec.ts(1 hunks)__tests__/statistics.spec.ts(3 hunks)index.d.ts(1 hunks)jest.config.js(2 hunks)lib/commons/config.ts(2 hunks)lib/commons/errors.ts(1 hunks)lib/constants/events.ts(1 hunks)lib/constants/outputs.js(0 hunks)lib/constants/outputs.ts(1 hunks)lib/constants/statuses.js(0 hunks)lib/constants/statuses.ts(1 hunks)lib/helpers.ts(4 hunks)lib/logger.js(0 hunks)lib/logger.ts(1 hunks)lib/publicReportingAPI.js(0 hunks)lib/publicReportingAPI.ts(1 hunks)lib/report-portal-client.ts(27 hunks)lib/rest.ts(6 hunks)lib/types.ts(1 hunks)package.json(3 hunks)statistics/client-id.ts(3 hunks)statistics/constants.ts(3 hunks)statistics/statistics.ts(3 hunks)tsconfig.json(1 hunks)
💤 Files with no reviewable changes (5)
- lib/logger.js
- lib/constants/outputs.js
- lib/constants/statuses.js
- tests/rest.spec.js
- lib/publicReportingAPI.js
🧰 Additional context used
🧬 Code graph analysis (9)
__tests__/outputs.spec.ts (1)
lib/constants/outputs.ts (1)
OUTPUT_TYPES(3-8)
__tests__/config.spec.ts (1)
lib/commons/config.ts (2)
getRequiredOption(83-83)getClientConfig(83-83)
lib/publicReportingAPI.ts (2)
lib/types.ts (1)
Attribute(1-5)lib/constants/events.ts (1)
EVENTS(1-9)
__tests__/rest.spec.ts (1)
lib/rest.ts (1)
RestClient(27-157)
lib/helpers.ts (2)
lib/rest.ts (2)
request(60-91)RequestMethod(25-25)lib/types.ts (1)
Attribute(1-5)
lib/rest.ts (1)
lib/logger.ts (1)
addLogger(4-43)
lib/commons/config.ts (3)
lib/constants/outputs.ts (1)
OUTPUT_TYPES(3-8)lib/commons/errors.ts (2)
ReportPortalRequiredOptionError(17-23)ReportPortalValidationError(9-15)lib/types.ts (1)
ClientConfig(7-26)
statistics/statistics.ts (1)
lib/types.ts (1)
AgentParams(28-31)
lib/report-portal-client.ts (3)
lib/types.ts (12)
ClientConfig(7-26)MapType(105-105)AgentParams(28-31)TempIdPromise(108-111)ItemObj(95-103)LaunchDataRQ(33-40)FinishExecutionRQ(42-45)MergeOptions(47-52)TestItemDataRQ(54-65)FinishTestItemRQ(67-81)SaveLogRQ(89-93)FileObj(83-87)lib/rest.ts (2)
RestClient(27-157)request(60-91)statistics/statistics.ts (1)
Statistics(14-59)
🪛 GitHub Check: test (20)
__tests__/publicReportingAPI.spec.ts
[warning] 1-1:
Using exported name 'PublicReportingAPI' as identifier for default export
__tests__/config.spec.ts
[warning] 29-29:
Unexpected any. Specify a different type
[warning] 76-76:
Unexpected any. Specify a different type
__tests__/rest.spec.ts
[warning] 46-46:
'client' is assigned a value but never used
[warning] 27-27:
'unauthorizedErrorMessage' is assigned a value but never used
[warning] 3-3:
'http' is defined but never used
[warning] 2-2:
'isEqual' is defined but never used
statistics/client-id.ts
[warning] 22-22:
Unexpected any. Specify a different type
__tests__/statistics.spec.ts
[warning] 51-51:
Unexpected any. Specify a different type
[warning] 69-69:
Unexpected any. Specify a different type
🪛 GitHub Check: test (18)
__tests__/publicReportingAPI.spec.ts
[warning] 1-1:
Using exported name 'PublicReportingAPI' as identifier for default export
__tests__/config.spec.ts
[warning] 29-29:
Unexpected any. Specify a different type
[warning] 76-76:
Unexpected any. Specify a different type
__tests__/rest.spec.ts
[warning] 46-46:
'client' is assigned a value but never used
[warning] 27-27:
'unauthorizedErrorMessage' is assigned a value but never used
[warning] 3-3:
'http' is defined but never used
[warning] 2-2:
'isEqual' is defined but never used
statistics/client-id.ts
[warning] 22-22:
Unexpected any. Specify a different type
__tests__/statistics.spec.ts
[warning] 51-51:
Unexpected any. Specify a different type
[warning] 69-69:
Unexpected any. Specify a different type
🪛 GitHub Check: test (16)
__tests__/publicReportingAPI.spec.ts
[warning] 1-1:
Using exported name 'PublicReportingAPI' as identifier for default export
__tests__/config.spec.ts
[warning] 29-29:
Unexpected any. Specify a different type
[warning] 76-76:
Unexpected any. Specify a different type
__tests__/rest.spec.ts
[warning] 46-46:
'client' is assigned a value but never used
[warning] 27-27:
'unauthorizedErrorMessage' is assigned a value but never used
[warning] 3-3:
'http' is defined but never used
[warning] 2-2:
'isEqual' is defined but never used
statistics/client-id.ts
[warning] 22-22:
Unexpected any. Specify a different type
__tests__/statistics.spec.ts
[warning] 51-51:
Unexpected any. Specify a different type
[warning] 69-69:
Unexpected any. Specify a different type
🪛 GitHub Check: test (22)
__tests__/publicReportingAPI.spec.ts
[warning] 1-1:
Using exported name 'PublicReportingAPI' as identifier for default export
__tests__/config.spec.ts
[warning] 29-29:
Unexpected any. Specify a different type
[warning] 76-76:
Unexpected any. Specify a different type
__tests__/rest.spec.ts
[warning] 46-46:
'client' is assigned a value but never used
[warning] 27-27:
'unauthorizedErrorMessage' is assigned a value but never used
[warning] 3-3:
'http' is defined but never used
[warning] 2-2:
'isEqual' is defined but never used
statistics/client-id.ts
[warning] 22-22:
Unexpected any. Specify a different type
__tests__/statistics.spec.ts
[warning] 51-51:
Unexpected any. Specify a different type
[warning] 69-69:
Unexpected any. Specify a different type
🪛 GitHub Check: test (14)
__tests__/publicReportingAPI.spec.ts
[warning] 1-1:
Using exported name 'PublicReportingAPI' as identifier for default import
__tests__/config.spec.ts
[warning] 29-29:
Unexpected any. Specify a different type
[warning] 76-76:
Unexpected any. Specify a different type
__tests__/rest.spec.ts
[warning] 46-46:
'client' is assigned a value but never used
[warning] 27-27:
'unauthorizedErrorMessage' is assigned a value but never used
[warning] 3-3:
'http' is defined but never used
[warning] 2-2:
'isEqual' is defined but never used
statistics/client-id.ts
[warning] 22-22:
Unexpected any. Specify a different type
__tests__/statistics.spec.ts
[warning] 51-51:
Unexpected any. Specify a different type
[warning] 69-69:
Unexpected any. Specify a different type
⏰ 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). (1)
- GitHub Check: test (16)
🔇 Additional comments (34)
index.d.ts (1)
26-26: LGTM - OptionalskippedIssueproperty added.The addition of the optional
skippedIssue?: booleanproperty to theReportPortalConfiginterface aligns with the broader TypeScript migration and provides consistent typing across the library.lib/constants/statuses.ts (1)
1-10: LGTM - Well-structured status enum migration.The conversion from CommonJS to a TypeScript enum with string values is well-executed. The enum values follow consistent naming conventions and the string values are appropriate for a reporting library.
lib/constants/events.ts (1)
1-9: LGTM - Clean ES module export migration.The conversion from CommonJS
module.exportsto ES moduleexport constis clean and maintains all event mappings. The naming convention for events (using 'rp:' prefix) is consistent.statistics/constants.ts (3)
1-3: LGTM - ES module imports migration.The conversion from
require()to ES module imports is consistent with the overall migration strategy.
20-31: LGTM - Explicit return type annotation added.The addition of
: string | nullreturn type annotation togetNodeVersion()improves type safety and makes the function's contract explicit.
35-48: LGTM - Clean named exports migration.The conversion from CommonJS
module.exportsto ES module named exports maintains all the original exports while providing better tree-shaking capabilities.jest.config.js (4)
2-2: LGTM - TypeScript support added.Adding 'ts' to
moduleFileExtensionsenables Jest to discover TypeScript test files.
3-3: LGTM - TypeScript test discovery enabled.The updated
testRegexpattern correctly includes both.test.tsand.spec.tsfiles alongside the existing JavaScript patterns.
15-17: LGTM - ts-jest transformer configured.The addition of the ts-jest transformer for
.tsfiles is correct and enables Jest to process TypeScript files properly.
5-5: Resolve coverage exclusion for logger.ts:lib/logger.tsexists and solely implements logging interceptors; excluding it from Jest coverage is intentional and no changes are required.lib/commons/errors.ts (3)
1-7: LGTM - Error class with explicit typing.The conversion to ES module export with explicit parameter typing (
message: string) improves type safety.
9-15: LGTM - Validation error class properly typed.The inheritance structure and explicit typing are correct.
17-23: LGTM - Required option error class properly typed.The parameter typing (
propertyName: string) and inheritance structure are well-implemented.package.json (8)
18-21: LGTM - Build output packaging.The change to publish
/builddirectory instead of source files is appropriate for the TypeScript compilation workflow.
22-23: LGTM - Updated entry points for built outputs.The main entry and types declaration now correctly point to the compiled outputs in the build directory.
24-61: Comprehensive exports map looks good.The exports configuration provides multiple entry points with consistent mapping to built outputs. The structure supports both CommonJS and ES module consumers across different environments.
62-80: LGTM - TypeScript declaration mapping.The
typesVersionsconfiguration correctly maps import paths to their corresponding.d.tsfiles in the build output.
81-81: LGTM - CommonJS module type declared.Explicitly setting
"type": "commonjs"clarifies the module system for the package.
83-83: Node.js version requirement updated appropriately.The update from
">=14.x"to">=16.x"aligns with modern Node.js support lifecycle and TypeScript requirements.
95-101: LGTM - TypeScript type dependencies added.The addition of
@types/*packages for glob, ini, jest, lodash, node, uniqid, and uuid provides proper typing support for the dependencies used in the codebase.
116-116: No update needed for ts-jest dependency: The project is already using the latest ts-jest version (^29.4.1).Likely an incorrect or invalid review comment.
__tests__/publicReportingAPI.spec.ts (1)
1-2: Confirm export shape of PublicReportingAPIIf publicReportingAPI exports a named symbol, prefer
import { PublicReportingAPI }; if it’s default, current import is fine. The linter warnings suggest a mismatch.__tests__/report-portal-client.spec.ts (4)
114-117: Good alignment with the updated TypeScript API.The test correctly verifies the new signature of
getRejectAnswerwhich now takes anErrorobject as the second parameter. The assertion properly checks that the promise rejects with the expected error.
186-191: Good test coverage for analytics control.The test properly verifies that the analytics event is not triggered when the
REPORTPORTAL_CLIENT_JS_NO_ANALYTICSenvironment variable is set to 'true'. This aligns well with privacy-conscious testing environments.
736-736: Test correctly updated for new startTestItem signature.The test properly uses the new signature where the first parameter is an object with
nameandcodeRefproperties instead oftestCaseId. This aligns with the TypeScript migration mentioned in the AI summary.
65-77: Ignore agent parameter suggestion
TheRPClientconstructor is defined asconstructor(options: Partial<ClientConfig>, agentParams?: AgentParams), making the second argument optional. All existing tests using the single-object argument remain valid and do not require updates.Likely an incorrect or invalid review comment.
lib/commons/config.ts (2)
38-40: Good defensive programming with null check.The explicit check for
nullalongside the type check prevents edge cases wheretypeof null === 'object'would pass. This is a robust validation approach.
71-71: Good addition of skippedIssue field.The addition of the
skippedIssuefield aligns with the TypeScript type definitions and maintains consistency with the broader migration effort.statistics/statistics.ts (2)
26-26: Good defensive handling of undefined INTERPRETER.Using
INTERPRETER || ''ensures that even ifINTERPRETERis undefined, the field will have a valid string value, preventing potential runtime issues.
56-56: Proper error type assertion for better error handling.The use of
(error as Error).messageis a good practice for handling unknown error types in TypeScript catch blocks.lib/rest.ts (1)
1-1: LGTM on the TS surface.The generic request, typed helpers, and ES module export look solid.
lib/report-portal-client.ts (3)
526-529: Good: skippedIssue handling correctly sets NOT_ISSUE for SKIPPEDThis aligns with expected RP behavior and avoids creating issues for skipped tests when disabled.
155-160: Good: analytics opt-out honors env var string 'true'Simple and predictable behavior.
185-205: Good: safe attribute augmentation and launch UUID printingSystem attribute merge looks correct; guarded function-type check avoids calling non-functions here.
| "import/prefer-default-export": 0, | ||
| "@typescript-eslint/naming-convention": 0, | ||
| "@typescript-eslint/dot-notation": 0, | ||
| "@typescript-eslint/ban-ts-ignore": 0, |
There was a problem hiding this comment.
Remove deprecated rule '@typescript-eslint/ban-ts-ignore' to prevent ESLint failure
This rule was removed years ago and will trigger “Definition for rule not found”.
Apply this diff:
- "@typescript-eslint/ban-ts-ignore": 0,📝 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.
| "@typescript-eslint/ban-ts-ignore": 0, |
🤖 Prompt for AI Agents
In .eslintrc around line 32, remove the deprecated rule entry
"@typescript-eslint/ban-ts-ignore": 0 because it no longer exists and causes
“Definition for rule not found” ESLint errors; either delete that line entirely
or replace it with the current equivalent (for example
"@typescript-eslint/ban-ts-comment": "off"/0 or another desired setting) to
prevent the ESLint failure.
| } catch (error) { | ||
| // don't throw the error up to not break the entire process | ||
| // eslint-disable-next-line no-console | ||
| console.dir(error); | ||
| } | ||
|
|
||
| // @ts-ignore | ||
| return calculatedOptions; |
There was a problem hiding this comment.
Error handling suppresses TypeScript checks.
The catch block logs errors but doesn't re-throw them, and the function returns a potentially undefined value with @ts-ignore. This could lead to runtime issues if calculatedOptions is undefined.
Consider either:
- Re-throwing the error after logging, or
- Returning a default/partial config object
} catch (error) {
// don't throw the error up to not break the entire process
// eslint-disable-next-line no-console
console.dir(error);
+ // Return minimal config or re-throw
+ throw error; // Option 1
+ // OR
+ return {
+ apiKey: '',
+ project: '',
+ endpoint: '',
+ launch: '',
+ } as ClientConfig; // Option 2 with defaults
}
- // @ts-ignore
return calculatedOptions;Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In lib/commons/config.ts around lines 73-80, the catch block only logs the error
then uses "@ts-ignore" to return calculatedOptions which may be undefined;
remove the "@ts-ignore" and handle the error explicitly: either re-throw the
caught error after logging (throw error) so callers must handle failures, or
construct and return a typed default/partial config object (e.g.,
defaultCalculatedOptions) ensuring the return type is satisfied; update the
function signature or callers if you choose to allow undefined, and keep the
console/logging for diagnostics before re-throwing or returning the safe
default.
| fs.open(filename, 'w', (err: NodeJS.ErrnoException | null) => { | ||
| if (err) { | ||
| throw err; | ||
| } | ||
| }); |
There was a problem hiding this comment.
Fix file descriptor leak when creating marker files.
fs.open allocates an FD that you never close. Switch to writeFile (auto-closes) or close the FD.
- fs.open(filename, 'w', (err: NodeJS.ErrnoException | null) => {
- if (err) {
- throw err;
- }
- });
+ fs.writeFile(filename, '', (err: NodeJS.ErrnoException | null) => {
+ if (err) throw err;
+ });Apply the same change to saveLaunchUuidToFile.
Also applies to: 101-105
🤖 Prompt for AI Agents
In lib/helpers.ts around lines 46-50 (and similarly lines 101-105), fs.open is
used to create marker files but the returned file descriptor is never closed,
leaking FDs; replace the fs.open call with fs.writeFile (or
fs.promises.writeFile) to create the file atomically and let Node auto-close the
handle, or if you must use fs.open, capture the fd and call fs.close (or use
fs.closeSync) in the callback with proper error handling; make the same
replacement for saveLaunchUuidToFile so neither path leaks file descriptors.
| this.logDebug(`Launches with UUIDs: ${launchUUIds} were successfully merged!`); | ||
| if (this.config.launchUuidPrint) { | ||
| this.config.launchUuidPrintOutput(response.uuid); | ||
| if ( | ||
| this.config.launchUuidPrint && | ||
| this.config.launchUuidPrintOutput && | ||
| typeof (this.config.launchUuidPrintOutput === 'function') | ||
| ) { | ||
| const uuid = response.uuid || 'STDOUT'; | ||
| // @ts-ignore | ||
| this.config.launchUuidPrintOutput(uuid); | ||
| } |
There was a problem hiding this comment.
Incorrect typeof check always truthy; can call string as function
typeof (this.config.launchUuidPrintOutput === 'function') evaluates "boolean", so the condition effectively ignores the function check and may attempt to call a string value, causing a runtime TypeError.
Apply:
- if (
- this.config.launchUuidPrint &&
- this.config.launchUuidPrintOutput &&
- typeof (this.config.launchUuidPrintOutput === 'function')
- ) {
- const uuid = response.uuid || 'STDOUT';
- // @ts-ignore
- this.config.launchUuidPrintOutput(uuid);
- }
+ if (
+ this.config.launchUuidPrint &&
+ typeof this.config.launchUuidPrintOutput === 'function'
+ ) {
+ const uuid = response.uuid ?? 'STDOUT';
+ this.config.launchUuidPrintOutput(uuid);
+ }This also removes the need for ts-ignore.
📝 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.
| this.logDebug(`Launches with UUIDs: ${launchUUIds} were successfully merged!`); | |
| if (this.config.launchUuidPrint) { | |
| this.config.launchUuidPrintOutput(response.uuid); | |
| if ( | |
| this.config.launchUuidPrint && | |
| this.config.launchUuidPrintOutput && | |
| typeof (this.config.launchUuidPrintOutput === 'function') | |
| ) { | |
| const uuid = response.uuid || 'STDOUT'; | |
| // @ts-ignore | |
| this.config.launchUuidPrintOutput(uuid); | |
| } | |
| this.logDebug(`Launches with UUIDs: ${launchUUIds} were successfully merged!`); | |
| if ( | |
| this.config.launchUuidPrint && | |
| typeof this.config.launchUuidPrintOutput === 'function' | |
| ) { | |
| const uuid = response.uuid ?? 'STDOUT'; | |
| this.config.launchUuidPrintOutput(uuid); | |
| } |
🤖 Prompt for AI Agents
In lib/report-portal-client.ts around lines 333 to 342, the typeof check is
written as typeof (this.config.launchUuidPrintOutput === 'function') which
evaluates the boolean expression instead of checking the type of the value;
change the condition to ensure launchUuidPrintOutput exists and is a function
(e.g. this.config.launchUuidPrintOutput && typeof
this.config.launchUuidPrintOutput === 'function'), then remove the // @ts-ignore
and call the function normally with the uuid variable.
| .catch((error: any) => { | ||
| this.logDebug(`Error save log with file: ${fileObj.name}`, error); | ||
| // eslint-disable-next-line no-console | ||
| console.dir(error); | ||
| }); | ||
| } |
There was a problem hiding this comment.
Swallowed errors in getRequestLogWithFile lead to false “success”
Catching and not rethrowing resolves the Promise with undefined; upstream saveLog then treats it as success.
Apply either of:
Option A (preferred): let errors bubble
- .catch((error: any) => {
- this.logDebug(`Error save log with file: ${fileObj.name}`, error);
- // eslint-disable-next-line no-console
- console.dir(error);
- });
+ );Option B: rethrow after logging
.catch((error: any) => {
this.logDebug(`Error save log with file: ${fileObj.name}`, error);
// eslint-disable-next-line no-console
console.dir(error);
+ throw 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.
| .catch((error: any) => { | |
| this.logDebug(`Error save log with file: ${fileObj.name}`, error); | |
| // eslint-disable-next-line no-console | |
| console.dir(error); | |
| }); | |
| } | |
| .catch((error: any) => { | |
| this.logDebug(`Error save log with file: ${fileObj.name}`, error); | |
| // eslint-disable-next-line no-console | |
| console.dir(error); | |
| throw error; | |
| }); | |
| } |
🤖 Prompt for AI Agents
In lib/report-portal-client.ts around lines 700 to 705, the catch block in
getRequestLogWithFile swallows errors by logging and not rethrowing, causing the
returned Promise to resolve as undefined and upstream saveLog to treat it as
success; fix by either removing the catch so errors bubble to the caller
(preferred) or, if logging is required, rethrow the caught error after logging
(e.g., log then throw error) so the Promise rejects and upstream can handle the
failure.
| import { v4 as uuidv4 } from 'uuid'; | ||
| import { ENCODING, CLIENT_ID_KEY, RP_FOLDER_PATH, RP_PROPERTIES_FILE_PATH } from './constants'; | ||
|
|
||
| const exists = util.promisify(fs.exists); |
There was a problem hiding this comment.
Do not promisify fs.exists; use fs.promises.access instead
fs.exists has a non-standard callback and is deprecated; promisifying it produces incorrect behavior. Replace with an access-based check.
Apply:
-const exists = util.promisify(fs.exists);
+const exists = async (path: string): Promise<boolean> => {
+ try {
+ await fs.promises.access(path);
+ return true;
+ } catch {
+ return 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.
| const exists = util.promisify(fs.exists); | |
| const exists = async (path: string): Promise<boolean> => { | |
| try { | |
| await fs.promises.access(path); | |
| return true; | |
| } catch { | |
| return false; | |
| } | |
| }; |
🤖 Prompt for AI Agents
In statistics/client-id.ts around line 7, replace the promisified fs.exists call
(const exists = util.promisify(fs.exists)) because fs.exists uses a non-standard
callback and is deprecated; instead use fs.promises.access to check file
existence. Update code to call await fs.promises.access(path, fs.constants.F_OK)
inside a try/catch and return true if access succeeds and false on ENOENT (or
any failure), or wrap that logic in an async helper like fileExists(path) that
returns a boolean.
| interface AgentParams { | ||
| name?: string; | ||
| version?: string; | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Duplicate type definition exists in lib/types.ts.
The AgentParams interface is defined here locally, but according to the relevant code snippets, there's already an AgentParams type exported from lib/types.ts. Consider using the shared type definition to avoid duplication and maintain consistency.
-interface AgentParams {
- name?: string;
- version?: string;
-}
+import { AgentParams } from '../lib/types';🏁 Script executed:
#!/bin/bash
# Verify that AgentParams in lib/types.ts has compatible structure
rg -A 3 "export type AgentParams" lib/types.tsLength of output: 65
Align optionality for AgentParams across modules
Imported AgentParams in lib/types.ts defines name and version as required, while the local interface marks them optional. Either update the shared type to use optional properties (name?: string; version?: string) or adjust all usages to supply both fields before replacing the local interface with the import.
🤖 Prompt for AI Agents
In statistics/statistics.ts around lines 5 to 8, the local AgentParams interface
marks name and version as optional while the imported type in lib/types.ts
requires them; align them by either updating lib/types.ts to make name and
version optional (name?: string; version?: string) or change all usages to
always provide both fields and then remove the local interface in favor of
importing the shared type—choose one approach and apply it consistently across
modules so the type definition matches usages.
| "module": "commonjs", | ||
| "target": "ES2015", | ||
| "baseUrl": ".", | ||
| "resolveJsonModule": true, |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
resolveJsonModule: ensure JSON files are copied to build and published
TypeScript will emit JSONs to outDir, but they must also be included in the published package. Verify that every imported JSON is present under build/ and covered by package.json "files"/"exports".
Run:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "JSON imports in TS sources:"
rg -nP --type=ts -C0 'from\s+["'\''][^"'\'']+\.json["'\'']|import\(.+\.json' || true
echo -e "\npackage.json publish config:"
jq '{name, version, type, main, module, exports, files}' package.json
echo -e "\nCheck if JSON assets are referenced under build/ in files/exports (manual review may be needed)."Length of output: 453
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "JSON imports in TS sources:"
rg -nP --type=ts -C0 'from\s+["'"'"'][^"'"'"']+\.json["'"'"']|import\(.+\.json' || true
echo -e "\nListing package.json 'files' and 'exports':"
jq '.files, .exports' package.json
echo -e "\nJSON files in build/ directory:"
find build -type f -name '*.json'Length of output: 1678
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "tsconfig.json content:"
sed -n '1,200p' tsconfig.jsonLength of output: 681
Copy imported JSON files into your build output and include them in the published package
Imported JSON modules (e.g. package.json in statistics/constants.ts, lib/helpers.ts, tests) aren’t emitted by TypeScript at runtime. Add a step (e.g. a glob-copy in your build script or bundler) to copy these JSON files into build/, and ensure your package.json “files” and “exports” cover them so consumers can resolve them.
🤖 Prompt for AI Agents
In tsconfig.json around line 16, resolveJsonModule is enabled but imported JSON
files aren’t emitted to the build output; update the build process to copy any
JSON imports into build/ (e.g., add a glob copy step in your build script or
configure your bundler to emit JSON), then update package.json “files” and
“exports” to include those JSON files/paths so published packages and consumers
can resolve them at runtime (ensure relative paths in code match the emitted
locations).
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
.github/workflows/CI-pipeline.yml (4)
42-49: Use npm cache and npm ci for faster, reproducible installs.Leverage setup-node cache and lockfile-friendly install.
- name: Setup Node.js uses: actions/setup-node@v4 with: - node-version: ${{ matrix.node }} + node-version: ${{ matrix.node }} + cache: 'npm' - name: Install dependencies - run: npm install + run: npm ci
50-58: Small CI hardening: set CI=true and keep tests non-interactive.This helps Jest/linters behave consistently in CI.
- name: Build the source code - run: npm run build + run: npm run build + env: + CI: true - name: Run lint - run: npm run lint + run: npm run lint + env: + CI: true - name: Run tests and check coverage - run: npm run test:coverage + run: npm run test:coverage -- --ci + env: + CI: true
31-33: Cancel superseded workflow runs to save CI minutes.Useful on active PRs with frequent pushes.
paths-ignore: - CHANGELOG.md +concurrency: + group: ${{ github.workflow }}-${{ github.ref }} + cancel-in-progress: true + jobs: test:
37-37: Align CI matrix withpackage.jsonengines and mark Node 24 experimental
Theengines.nodefield specifies>=16.x, so include16.xin your matrix. Use.xfor patch updates, disablefail-fast, and continue on error for experimental Node 24:strategy: - matrix: - node: [16, 18, 20, 22, 24] + fail-fast: false + matrix: + node: [16.x, 18.x, 20.x, 22.x] + include: + - node: 24.x + experimental: true + continue-on-error: ${{ matrix.experimental == true }}
|
Great work on this! I'm looking forward to getting these changes soon. Do you have an ETA for when this might be merged? |
3 participants
Summary by CodeRabbit
Breaking Changes
New Features
Tests & Tooling