EPMRPP-84449 || Added 'skippedIssue' #246
Conversation
WalkthroughAdds an optional config flag Changes
Sequence Diagram(s)sequenceDiagram
participant Runner as Test Runner
participant Client as ReportPortal Client
participant RP as ReportPortal API
Note over Runner,Client: A test item finishes with status = SKIPPED
Runner->>Client: finishTestItem(status: "SKIPPED")
alt config.skippedIsNotIssue === true
Client->>Client: attach issue: { issueType: "NOT_ISSUE" }
Client->>RP: finishTestItemPromiseStart(payload with issue)
else
Client->>Client: leave payload without issue
Client->>RP: finishTestItemPromiseStart(payload without issue)
end
RP-->>Client: ACK/response
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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: 3
🧹 Nitpick comments (2)
lib/report-portal-client.js (1)
614-615: Consider renaming the configuration option for clarity.The naming
skippedIssue: falseto mean "add NOT_ISSUE to skipped items" is counter-intuitive. The flag name suggests it controls whether skipped items should have issues, but the logic is inverted. Consider renaming to something likeattachNotIssueToSkippedor inverting the logic to makeskippedIssue: truemean "add issue annotation."Current behavior:
skippedIssue: false→ adds NOT_ISSUEskippedIssue: true→ doesn't add NOT_ISSUEskippedIssue: undefined→ doesn't add NOT_ISSUEThis could be clearer with either:
- Rename to
attachNotIssueToSkipped: true- Or invert logic:
skippedIssue: trueadds NOT_ISSUE (meaning "treat skipped as issue")__tests__/report-portal-client.spec.js (1)
879-962: Consider adding a test case for undefinedskippedIssue.The tests cover
skippedIssue: falseandskippedIssue: true, but don't test the default behavior whenskippedIssueis undefined. This would help document and ensure the expected default behavior.Add a third test case:
it('should not add NOT_ISSUE when status is SKIPPED and skippedIssue is undefined', function (done) { const mockClient = new RPClient( { apiKey: 'test', endpoint: 'https://reportportal-stub-url', launch: 'test launch', project: 'test project', // skippedIssue is undefined (not set) }, { name: 'test', version: '1.0.0' }, ); const spyFinishTestItemPromiseStart = jest .spyOn(mockClient, 'finishTestItemPromiseStart') .mockImplementation(() => {}); mockClient.map = { testItemId: { children: [], finishSend: false, promiseFinish: Promise.resolve(), resolveFinish: () => {}, }, }; const finishTestItemRQ = { status: 'skipped', }; mockClient.finishTestItem('testItemId', finishTestItemRQ); setTimeout(() => { expect(spyFinishTestItemPromiseStart).not.toHaveBeenCalledWith( expect.any(Object), 'testItemId', expect.objectContaining({ issue: expect.anything(), }), ); done(); }, 50); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
__tests__/report-portal-client.spec.js(1 hunks)index.d.ts(1 hunks)lib/commons/config.js(1 hunks)lib/report-portal-client.js(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
lib/report-portal-client.js (1)
lib/constants/statuses.js (1)
RP_STATUSES(1-10)
lib/commons/config.js (1)
__tests__/rest.spec.js (1)
options(8-20)
🪛 GitHub Actions: CI-pipeline
__tests__/report-portal-client.spec.js
[error] 880-880: prettier/prettier: Insert ⏎······
[warning] 37-37: Unexpected console statement no-console
[warning] 51-51: Unexpected console statement no-console
[warning] 65-65: Unexpected console statement no-console
🪛 GitHub Check: test (16)
__tests__/report-portal-client.spec.js
[failure] 895-895:
Unexpected empty arrow function 'resolveFinish'
[failure] 888-888:
Unexpected empty arrow function
[failure] 888-888:
Replace .spyOn(mockClient,·'finishTestItemPromiseStart') with ⏎······.spyOn(mockClient,·'finishTestItemPromiseStart')⏎······
[failure] 886-886:
Replace ····},·{·name:·'test',·version:·'1.0.0'·} with ······},⏎······{·name:·'test',·version:·'1.0.0'·},⏎····
[failure] 885-885:
Insert ··
[failure] 884-884:
Insert ··
[failure] 883-883:
Replace ······ with ········
[failure] 882-882:
Insert ··
[failure] 881-881:
Insert ··
[failure] 880-880:
Insert ⏎······
🪛 GitHub Check: test (18)
__tests__/report-portal-client.spec.js
[failure] 895-895:
Unexpected empty arrow function 'resolveFinish'
[failure] 888-888:
Unexpected empty arrow function
[failure] 888-888:
Replace .spyOn(mockClient,·'finishTestItemPromiseStart') with ⏎······.spyOn(mockClient,·'finishTestItemPromiseStart')⏎······
[failure] 886-886:
Replace ····},·{·name:·'test',·version:·'1.0.0'·} with ······},⏎······{·name:·'test',·version:·'1.0.0'·},⏎····
[failure] 885-885:
Insert ··
[failure] 884-884:
Insert ··
[failure] 883-883:
Replace ······ with ········
[failure] 882-882:
Insert ··
[failure] 881-881:
Insert ··
[failure] 880-880:
Insert ⏎······
🪛 GitHub Check: test (20)
__tests__/report-portal-client.spec.js
[failure] 895-895:
Unexpected empty arrow function 'resolveFinish'
[failure] 888-888:
Unexpected empty arrow function
[failure] 888-888:
Replace .spyOn(mockClient,·'finishTestItemPromiseStart') with ⏎······.spyOn(mockClient,·'finishTestItemPromiseStart')⏎······
[failure] 886-886:
Replace ····},·{·name:·'test',·version:·'1.0.0'·} with ······},⏎······{·name:·'test',·version:·'1.0.0'·},⏎····
[failure] 885-885:
Insert ··
[failure] 884-884:
Insert ··
[failure] 883-883:
Replace ······ with ········
[failure] 882-882:
Insert ··
[failure] 881-881:
Insert ··
[failure] 880-880:
Insert ⏎······
🪛 GitHub Check: test (22)
__tests__/report-portal-client.spec.js
[failure] 895-895:
Unexpected empty arrow function 'resolveFinish'
[failure] 888-888:
Unexpected empty arrow function
[failure] 888-888:
Replace .spyOn(mockClient,·'finishTestItemPromiseStart') with ⏎······.spyOn(mockClient,·'finishTestItemPromiseStart')⏎······
[failure] 886-886:
Replace ····},·{·name:·'test',·version:·'1.0.0'·} with ······},⏎······{·name:·'test',·version:·'1.0.0'·},⏎····
[failure] 885-885:
Insert ··
[failure] 884-884:
Insert ··
[failure] 883-883:
Replace ······ with ········
[failure] 882-882:
Insert ··
[failure] 881-881:
Insert ··
[failure] 880-880:
Insert ⏎······
🔇 Additional comments (1)
lib/commons/config.js (1)
118-118: LGTM!The configuration option is correctly passed through to the calculated options, following the same pattern as other configuration fields.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
__tests__/report-portal-client.spec.js (1)
879-971: Based on my verification, I found that:
- The code snippet matches exactly at lines 879-971
- The referenced "static_analysis_hints/pipeline_failures" does not exist in the repository
- Airbnb ESLint config does not flag empty arrow functions as errors by default
- However, the patterns ARE inconsistent with existing code in the same test file:
- Existing code uses
.mockImplementation()without arguments (lines 33, 47, etc.)- Existing code uses
jest.fn()forresolveFinish(lines 463, 466)The original review comment incorrectly frames consistency improvements as ESLint errors. The suggestions themselves are valid for improving code consistency with existing patterns, but they should not be presented as lint violations.
Rewritten Review Comment:
Align mock patterns with existing test conventions for consistency
The new tests use inconsistent mock patterns compared to the rest of the file:
.mockImplementation(() => {})differs from the established.mockImplementation()pattern used in 20+ other testsresolveFinish: () => {}differs from thejest.fn()pattern used elsewhere for this property (lines 463, 466)Update both test blocks to match existing conventions:
- const spyFinishTestItemPromiseStart = jest - .spyOn(mockClient, 'finishTestItemPromiseStart') - .mockImplementation(() => {}); + const spyFinishTestItemPromiseStart = jest + .spyOn(mockClient, 'finishTestItemPromiseStart') + .mockImplementation(); @@ - resolveFinish: () => {}, + resolveFinish: jest.fn(),Apply the same changes to the second test block (lines 939 and 946).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
__tests__/report-portal-client.spec.js(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: CI-pipeline
__tests__/report-portal-client.spec.js
[warning] 37-37: Unexpected console statement. ESLint: no-console.
[warning] 51-51: Unexpected console statement. ESLint: no-console.
[warning] 65-65: Unexpected console statement. ESLint: no-console.
[error] 893-893: Unexpected empty arrow function. @typescript-eslint/no-empty-function.
🪛 GitHub Check: test (16)
__tests__/report-portal-client.spec.js
[failure] 944-944:
Unexpected empty arrow function 'resolveFinish'
[failure] 937-937:
Unexpected empty arrow function
[failure] 900-900:
Unexpected empty arrow function 'resolveFinish'
[failure] 893-893:
Unexpected empty arrow function
🪛 GitHub Check: test (18)
__tests__/report-portal-client.spec.js
[failure] 944-944:
Unexpected empty arrow function 'resolveFinish'
[failure] 937-937:
Unexpected empty arrow function
[failure] 900-900:
Unexpected empty arrow function 'resolveFinish'
[failure] 893-893:
Unexpected empty arrow function
🪛 GitHub Check: test (20)
__tests__/report-portal-client.spec.js
[failure] 944-944:
Unexpected empty arrow function 'resolveFinish'
[failure] 937-937:
Unexpected empty arrow function
[failure] 900-900:
Unexpected empty arrow function 'resolveFinish'
[failure] 893-893:
Unexpected empty arrow function
🪛 GitHub Check: test (22)
__tests__/report-portal-client.spec.js
[failure] 944-944:
Unexpected empty arrow function 'resolveFinish'
[failure] 937-937:
Unexpected empty arrow function
[failure] 900-900:
Unexpected empty arrow function 'resolveFinish'
[failure] 893-893:
Unexpected empty arrow function
There was a problem hiding this comment.
Please also update systemAttributes calculation for true value in accordance with current implementation in agents.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
index.d.ts (1)
146-163: DocumentskippedIsNotIssuesemantics (and consider a deprecated alias forskippedIssue).The new flag is correctly wired into the config type, but its behavior isn’t obvious from the name alone (auto‑mark SKIPPED items as NOT_ISSUE and add the
skippedIssue="false"system attribute). It would help TS consumers if this were documented, and since runtime still supports the legacyskippedIssueoption (per tests), you may also want to keep it in the type as a deprecated alias for backward compatibility.Example:
export interface ReportPortalConfig { @@ restClientConfig?: RestClientConfig; token?: string; - skippedIsNotIssue?: boolean; + /** + * When `true`, SKIPPED test items without an explicit `issue` are + * automatically annotated with `{ issueType: 'NOT_ISSUE' }`, and the + * launch gets a system attribute `skippedIssue="false"` for backward + * compatibility. + * When `false` or omitted, skipped items are not auto‑annotated. + */ + skippedIsNotIssue?: boolean; + /** + * @deprecated Use `skippedIsNotIssue` instead. + * Legacy flag preserved for backward compatibility. + */ + // skippedIssue?: boolean;(You can uncomment the legacy prop if you decide to expose it in the typings.)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
__tests__/report-portal-client.spec.js(1 hunks)index.d.ts(1 hunks)lib/commons/config.js(1 hunks)lib/report-portal-client.js(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- lib/report-portal-client.js
- lib/commons/config.js
🧰 Additional context used
🪛 GitHub Actions: CI-pipeline
__tests__/report-portal-client.spec.js
[error] 923-923: Exceeded timeout of 5000 ms for a test while waiting for done() to be called.
maria-hambardzumian
deleted the
feature/EPMRPP-84449-added-skippedIssue
branch
2 participants
Summary by CodeRabbit
New Features
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.