RHDHBUGS-3057: Refactor e2e tests for the scorecard plugin#3245
Conversation
…functions Signed-off-by: Ihor Mykhno <imykhno@redhat.com>
imykhno
requested review from
a team,
Eswaraiahsapram,
christoph-jerolimov and
its-mitesh-kumar
as code owners
Code Review by Qodo
1.
|
Changed Packages
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3245 +/- ##
=======================================
Coverage 53.23% 53.24%
=======================================
Files 2413 2414 +1
Lines 86358 86367 +9
Branches 23912 23907 -5
=======================================
+ Hits 45974 45982 +8
- Misses 38907 38908 +1
Partials 1477 1477
*This pull request uses carry forward flags. Click here to find out more. Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
…nts and improve widget handling Signed-off-by: Ihor Mykhno <imykhno@redhat.com>
Review Summary by QodoRefactor scorecard e2e tests with aggregation utilities and improved test structure
WalkthroughsDescription• Refactored e2e test structure with new aggregation constants and utility functions • Reorganized constants from homepageWidgetTitles.ts to aggregations.ts with improved naming • Extracted common test setup logic into reusable homepageWidgetUtils.ts helper functions • Enhanced API utilities with aggregation response waiting and validation capabilities • Improved test organization by grouping related aggregation tests with shared setup Diagramflowchart LR
A["Old Constants<br/>homepageWidgetTitles.ts"] -->|Migrate & Rename| B["New Constants<br/>aggregations.ts"]
C["Scattered Test Setup<br/>Code"] -->|Extract| D["Utility Functions<br/>homepageWidgetUtils.ts"]
E["Basic API Utils<br/>apiUtils.ts"] -->|Enhance| F["Aggregation Response<br/>Waiting & Validation"]
B --> G["Cleaner Test Code<br/>scorecard.test.ts"]
D --> G
F --> G
File Changes1. workspaces/scorecard/packages/app-legacy/e2e-tests/constants/aggregations.ts
|
Signed-off-by: Ihor Mykhno <imykhno@redhat.com>
imykhno
force-pushed
the
scorecard/implement-e2e-tests
branch
from
ca1706a to
1d79f52
Compare
|
/publish |
|
/fs-review |
2 similar comments
|
/fs-review |
|
/fs-review |
ReviewFindingsHigh
Low
Info
Previous runReviewFindingsMedium
Low
Info
Previous run (2)ReviewFindingsHigh
Low
Info
Previous run (3)ReviewFindingsMedium
Low
Previous run (4)ReviewFindingsHigh
Medium
Low
Info
|
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
| }); | ||
| } | ||
|
|
||
| export async function mockAggregationNoDataFound(page: Page): Promise<void> { |
There was a problem hiding this comment.
[high] logic-error
mockAggregationNoDataFound intercepts /api/scorecard/aggregations/ which matches metadata URLs. For metadata URLs, url.split('/').pop() returns 'metadata' which doesn't match any aggregation ID, falling through to the 404 branch. Card titles/descriptions fail to load, producing an error UI instead of the intended 'no data found' state. The existing mockHomepageAggregationsPermissionDenied correctly handles this by checking url.includes('/metadata').
Suggested fix: Add a metadata check at the top of the route handler: if url.includes('/metadata'), return 200 with the appropriate metadata response using the existing aggregationMetadataForRequestUrl helper.
| function isAggregationDataUrl(url: string, aggregationId: string): boolean { | ||
| return ( | ||
| url.includes(`/api/scorecard/aggregations/${aggregationId}`) && | ||
| !url.includes('/metadata') |
There was a problem hiding this comment.
[low] edge-case
isAggregationDataUrl uses url.split('/').pop() which would include query parameters if present, causing match failures.
| response: object; | ||
| status?: number; | ||
| }; | ||
|
|
There was a problem hiding this comment.
[low] naming-convention
addWidgets is exported but only used internally by setupHomepageAggregationCard within the same file. Consider removing the export.
…get handling Signed-off-by: Ihor Mykhno <imykhno@redhat.com>
fullsend-ai-review
Bot
added
the
requires-manual-review
There was a problem hiding this comment.
Thank you @imykhno for this nice refactoring, I have only a couple of small comments, otherwise looks nice.
Can you please also confirm that all of these comments are fixed by the PR or are tracked in other issue or were fixed before?
- #2923 (comment)
- #2923 (comment)
- #2923 (comment)
- #2923 (comment) -> see https://github.com/redhat-developer/rhdh-plugins/pull/3245/changes#r3374295054
Are we testing when some entities are reporting errors in aggregation scorecards? E.g we show x/y in the aggregation card and some rows in drill down do not show status. If not, we can implement those as part of https://redhat.atlassian.net/browse/RHIDP-14421
Signed-off-by: Ihor Mykhno <imykhno@redhat.com>
|
🤖 Finished Review · ✅ Success · Started 3:47 PM UTC · Completed 3:57 PM UTC |
| @@ -129,11 +111,6 @@ test.describe('Scorecard Plugin Tests', () => { | |||
| await page?.context()?.close(); | |||
There was a problem hiding this comment.
[low] test-weakened
The afterEach block that called page.unroute() was removed. Route handlers accumulate on the shared page instance across tests.
| } | ||
|
|
||
| await homePage.saveChanges(); | ||
| } |
There was a problem hiding this comment.
[low] pattern-violation
Dynamic string key from Object.keys() used to index an as-const object bypasses TypeScript type safety.
fullsend-ai-review
Bot
removed
the
requires-manual-review
…avior Signed-off-by: Ihor Mykhno <imykhno@redhat.com>
|
🤖 Review · Started 5:08 PM UTC |
Comment:
Answers:
Comment:
Answers:
CC @dzemanov |
fullsend-ai-review
Bot
added
the
requires-manual-review
|
🤖 Finished Review · ✅ Success · Started 5:08 PM UTC · Completed 5:19 PM UTC |
…gation config Signed-off-by: Ihor Mykhno <imykhno@redhat.com>
|
|
🤖 Finished Review · ✅ Success · Started 9:44 AM UTC · Completed 9:56 AM UTC |
| } else if ( | ||
| cardName === AGGREGATED_CARDS_WIDGET_TITLES.withOpenPrsWeightedKpi | ||
| ) { | ||
| } else if (cardName === AGGREGATED_CARDS_WIDGET_TITLES.openPrsWeightedKpi) { |
There was a problem hiding this comment.
[high] logic-error
Property AGGREGATED_CARDS_WIDGET_TITLES.openPrsWeightedKpi does not exist on the new AGGREGATED_CARDS_WIDGET_TITLES object defined in aggregations.ts. The new object uses key gitHubOpenPrsWeightedKpi. Accessing .openPrsWeightedKpi evaluates to undefined, so the else if branch will never match and the weighted KPI card name will fall through to the generic else branch.
Suggested fix: Change AGGREGATED_CARDS_WIDGET_TITLES.openPrsWeightedKpi to AGGREGATED_CARDS_WIDGET_TITLES.gitHubOpenPrsWeightedKpi.
| @@ -129,11 +114,6 @@ test.describe('Scorecard Plugin Tests', () => { | |||
| await page?.context()?.close(); | |||
There was a problem hiding this comment.
[low] test-isolation
The test.afterEach that called page.unroute() was removed. Route handlers accumulate without cleanup, making the test suite slightly fragile to reordering.
Suggested fix: Consider calling page.unroute(route) before page.route(route, ...) inside mockApiResponse, or restore the afterEach cleanup.
| export const AGGREGATED_CARDS_WIDGET_TITLES = { | ||
| jiraMetricId: 'Scorecard: With deprecated metricId property (Jira)', | ||
| githubMetricId: 'Scorecard: With default aggregation config (GitHub)', | ||
| githubOpenPrsKpi: 'Scorecard: GitHub open PRs', |
There was a problem hiding this comment.
[low] naming-convention
Inconsistent casing for the GitHub prefix: githubMetricId and githubOpenPrsKpi use lowercase github, while gitHubOpenPrsWeightedKpi uses camelCase gitHub. Same inconsistency in scorecardResponseUtils.ts.
Suggested fix: Standardize to github (lowercase) prefix across all new identifiers.
| @@ -462,394 +441,419 @@ test.describe('Scorecard Plugin Tests', () => { | |||
| } | |||
There was a problem hiding this comment.
[low] coverage-reduced
The weighted KPI drill-down test and accessibility test were removed without full replacement. The new drill-down link test only verifies navigation, not drill-down card content or entity table.
fullsend-ai-review
Bot
removed
the
requires-manual-review
3 participants
Hey, I just made a Pull Request!
This refactoring follows the merge of #2923. The expectation was that tests will be reviewed and added those tests that are missing to test scorecard aggregation card customization. Additionally, this PR incorporates the feedback and comments raised during the review of #2923.
PR is for:
✔️ Checklist