[KFLUXUI-1070] refactor(e2e-tests/utils/Applications): use expect() in checkPipelineStatuses#711
Open
rrosatti wants to merge 4 commits intokonflux-ci:mainfrom
Open
Conversation
Contributor
|
Note
|
| Cohort / File(s) | Summary |
|---|---|
E2E test usage e2e-tests/tests/basic-happy-path.spec.ts |
Calls ComponentDetailsPage.verifyPipelineRunIsVisible(\${componentName}-on-pull`)` after opening a component. |
Applications utility e2e-tests/utils/Applications.ts |
Replaced manual DOM parsing with cy.get(pipelinerunsTabPO.pipelineRunRow(...), { timeout: 90000 }) and a single text-contains assertion; added UIhelperPO import and simplified timeout handling. |
Page objects — selectors e2e-tests/support/pageObjects/pages-po.ts |
Added compActivityPipelinerunsTabPO with clickTab and extended pipelinerunsTabPO with pipelineRunRow(runNamePrefix) selector helper. |
Component details page object e2e-tests/support/pages/ComponentDetailsPage.ts |
Added imports and a new public static method verifyPipelineRunIsVisible(plrName: string) that navigates Activity → Pipeline Runs, waits for the pipeline-run row (long timeout), asserts visibility, then returns to Details. |
Sequence Diagram(s)
(omitted)
Estimated code review effort
🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
- KFLUXUI-863 [e2e] enhance logic to to check plr status #583: Refactors pipeline-run status checking in
e2e-tests/utils/Applications.tsand touches related selectors/utilities. - feat( KFLUXUI-63): enable vulnerability test #556: Updates
pipelinerunsTabPOand pipeline-run related page-object helpers; overlaps with selector additions.
Suggested reviewers
- sahil143
- testcara
- Katka92
- janaki29
- abhinandan13jan
Poem
A tiny check on a pipeline row,
Tabs clicked where the e2e winds blow,
Page objects aligned, assertions bright,
CI hums softly into the night. 🚀
🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 inconclusive)
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Title check | ❓ Inconclusive | The title focuses on refactoring checkPipelineStatuses, but the PR also adds pipeline run verification logic and new page object selectors—significant changes not captured in the title. | Consider revising the title to reflect the full scope, such as 'refactor checkPipelineStatuses and add pipeline run verification' or clarify if the title should emphasize only the primary refactor. |
✅ Passed checks (2 passed)
| Check name | Status | Explanation |
|---|---|---|
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
| Docstring Coverage | ✅ Passed | No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. |
✏️ Tip: You can configure your own custom pre-merge checks in the settings.
✨ Finishing Touches
- 📝 Generate docstrings (stacked PR)
- 📝 Generate docstrings (commit on current branch)
🧪 Generate unit tests (beta)
- Create PR with unit tests
- Post copyable unit tests in a comment
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 help to get the list of available commands and usage tips.
Contributor
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #711 +/- ##
==========================================
- Coverage 87.23% 86.63% -0.60%
==========================================
Files 764 764
Lines 58225 58225
Branches 5658 5656 -2
==========================================
- Hits 50792 50444 -348
- Misses 7376 7602 +226
- Partials 57 179 +122
Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
rrosatti
force-pushed
the
KFLUXUI-1070/e2e-fix-cancelled-plr-check
branch
from
2f036ab to
8b61b9d
Compare
rrosatti
changed the title
rrosatti
commented
Feb 18, 2026
rrosatti
changed the title
rrosatti
requested review from
JoaoPedroPP,
Katka92,
StanislavJochman,
abhinandan13jan,
janaki29,
marcin-michal,
milantaky,
rakshett,
sahil143 and
testcara
and removed request for
a team
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
e2e-tests/utils/Applications.ts (1)
49-55: Redundantreturnin wrapper methods.
checkPipelineStatusesno longer returns a meaningful value (void), so thereturnin both wrappers is a no-op. Safe to remove for clarity.♻️ Proposed cleanup
static checkPipelineIsCancellingOrCancelled(componentName: string) { - return this.checkPipelineStatuses(componentName, ['Cancelling', 'Cancelled']); + this.checkPipelineStatuses(componentName, ['Cancelling', 'Cancelled']); } static checkPipelineIsCancelled(componentName: string) { - return this.checkPipelineStatuses(componentName, ['Cancelled']); + this.checkPipelineStatuses(componentName, ['Cancelled']); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e-tests/utils/Applications.ts` around lines 49 - 55, Both wrapper methods checkPipelineIsCancellingOrCancelled and checkPipelineIsCancelled are using "return" when calling checkPipelineStatuses, but checkPipelineStatuses is void so the return is redundant; remove the "return" keyword and simply call this.checkPipelineStatuses(componentName, ['Cancelling','Cancelled']) and this.checkPipelineStatuses(componentName, ['Cancelled']) respectively (preserve method names and signatures in class Applications).e2e-tests/tests/basic-happy-path.spec.ts (1)
102-104: Hardcodedcy.wait(30000)— consider a deterministic wait.A fixed 30-second sleep will unconditionally slow every test run. If there's a detectable UI condition that signals readiness (e.g., the PR modal closing, a loading spinner disappearing, or a specific element becoming visible/hidden), a
cy.get(...).should(...)wait would be both faster on average and more reliable under varying CI load. At a minimum, extracting the value to a named constant (e.g.,const MERGE_PR_STABILIZATION_WAIT_MS = 30_000) co-locates the intent with the other existing wait constants and makes the magic number easier to locate and adjust.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e-tests/tests/basic-happy-path.spec.ts` around lines 102 - 104, Replace the hardcoded cy.wait(30000) with a deterministic wait or at least a named constant: identify the UI condition that indicates readiness (e.g., PR modal close, spinner disappearance, or target element visibility) and use cy.get(...).should(...) to wait for that condition instead of a fixed sleep; if a deterministic selector can't be used immediately, replace cy.wait(30000) with a constant like MERGE_PR_STABILIZATION_WAIT_MS = 30_000 and reference that constant where cy.wait is called to document intent and make tuning easier (locate the existing cy.wait call in the test and update it).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@e2e-tests/tests/basic-happy-path.spec.ts`:
- Around line 102-104: Replace the hardcoded cy.wait(30000) with a deterministic
wait or at least a named constant: identify the UI condition that indicates
readiness (e.g., PR modal close, spinner disappearance, or target element
visibility) and use cy.get(...).should(...) to wait for that condition instead
of a fixed sleep; if a deterministic selector can't be used immediately, replace
cy.wait(30000) with a constant like MERGE_PR_STABILIZATION_WAIT_MS = 30_000 and
reference that constant where cy.wait is called to document intent and make
tuning easier (locate the existing cy.wait call in the test and update it).
In `@e2e-tests/utils/Applications.ts`:
- Around line 49-55: Both wrapper methods checkPipelineIsCancellingOrCancelled
and checkPipelineIsCancelled are using "return" when calling
checkPipelineStatuses, but checkPipelineStatuses is void so the return is
redundant; remove the "return" keyword and simply call
this.checkPipelineStatuses(componentName, ['Cancelling','Cancelled']) and
this.checkPipelineStatuses(componentName, ['Cancelled']) respectively (preserve
method names and signatures in class Applications).
dad88ee to
a711f38
Compare
Katka92
reviewed
Feb 19, 2026
Katka92
reviewed
Feb 19, 2026
e2e-tests/utils/Applications.ts
Outdated
| cy.contains(UIhelperPO.tableRow('Pipeline run List'), plrName, { | ||
| // extended timeout: GitHub synchronization can occasionally take some time, | ||
| // which causes PR creation to take longer. | ||
| timeout: 1200000, // 20min |
Member
There was a problem hiding this comment.
Is this timeout based on your experience? 20 minutes is crazy high for a pipeline creation. I think we don't want users to be waiting 20 minutes for a pipeline to be created.
Member
Author
There was a problem hiding this comment.
as we discussed in Slack, I've observed it taking from a few seconds up to ~15min. So, I just rounded up to 20min to be sure 😄
rrosatti
force-pushed
the
KFLUXUI-1070/e2e-fix-cancelled-plr-check
branch
2 times, most recently
from
0257f18 to
f80ee64
Compare
…Statuses Use Cypress expect() callback instead of returning true/false. Assisted-by: Cursor
Update e2e test to check if the -on-pull pipeline run is visible before merging the onboarding PR. Assisted-by: Cursor
rrosatti
force-pushed
the
KFLUXUI-1070/e2e-fix-cancelled-plr-check
branch
from
f80ee64 to
a6bb4e1
Compare
rrosatti
requested review from
JoaoPedroPP,
Katka92,
StanislavJochman,
abhinandan13jan,
janaki29,
marcin-michal,
milantaky,
rakshett,
sahil143 and
testcara
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@e2e-tests/support/pages/ComponentDetailsPage.ts`:
- Around line 22-26: The test currently uses
cy.contains(UIhelperPO.tableRow('Pipeline run List'), plrName, { timeout:
1200000 }) which only asserts existence; update the verifyPipelineRunIsVisible
step to chain .should('be.visible') to that cy.contains call so it asserts
actual visibility; locate the verifyPipelineRunIsVisible usage (or the block
using cy.contains with UIhelperPO.tableRow('Pipeline run List') and plrName) and
append .should('be.visible') after the timeout options.
In `@e2e-tests/utils/Applications.ts`:
- Around line 27-37: The code is incorrectly casting an options object onto the
.should(...) call which only accepts a callback; remove the "as { timeout:
number }" argument and the trailing options object from the .should invocation
so the call is just .should(($row) => { ... }); keep the timeout on cy.get(...)
(already set to 90000) so retries still occur; update the block around
pipelinerunsTabPO.pipelineRunRow(pipelineRunName) and the statuses
check/expectation to rely on Cypress's built-in retry behavior without the bogus
type assertion.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
e2e-tests/support/pageObjects/pages-po.tse2e-tests/support/pages/ComponentDetailsPage.tse2e-tests/tests/basic-happy-path.spec.tse2e-tests/utils/Applications.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
e2e-tests/utils/Applications.ts (1)
32-32: Nitpick:...is always appended even when text is shorter than 100 chars.- `Pipeline run row should show one of [${statuses.join(', ')}]; got: "${text.trim().slice(0, 100)}..."`, + `Pipeline run row should show one of [${statuses.join(', ')}]; got: "${text.trim().length > 100 ? text.trim().slice(0, 100) + '...' : text.trim()}"`,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e-tests/utils/Applications.ts` at line 32, The assertion message always appends "..." even when the captured text is shorter than 100 chars; change the message construction so you create a snippet from text.trim().slice(0, 100) and only append "..." when text.trim().length > 100 (use the existing text variable and the same template string context that builds `Pipeline run row should show one of [...]`).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@e2e-tests/utils/Applications.ts`:
- Around line 26-29: The test is vulnerable to matching multiple pipeline run
rows because pipelineRunRow uses a starts-with selector; update the assertion to
target a single row (e.g., the most-recent row) by applying a single-element
selection before reading text — locate the cy.get(...) that uses
pipelineRunRow(pipelineRunName) and call a single-row selector like .first() (or
otherwise narrow the selector to an exact match) so the subsequent text/includes
check against statuses inspects only the intended pipelineRunName row.
---
Nitpick comments:
In `@e2e-tests/utils/Applications.ts`:
- Line 32: The assertion message always appends "..." even when the captured
text is shorter than 100 chars; change the message construction so you create a
snippet from text.trim().slice(0, 100) and only append "..." when
text.trim().length > 100 (use the existing text variable and the same template
string context that builds `Pipeline run row should show one of [...]`).
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
e2e-tests/support/pages/ComponentDetailsPage.tse2e-tests/utils/Applications.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- e2e-tests/support/pages/ComponentDetailsPage.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes
Fixes https://issues.redhat.com/browse/KFLUXUI-1070
Description
In this PR we're fixing the logic in the
checkPipelineStatusesfunction, so it uses Cypressexpect()callback instead of returningtrue/false. It seems Cypress would not fail/throw if it returns false.Also, we are adding a logic to check if the -on-pull pipeline run is visible before merging the onboarding PR. It's a proposed approach to prevent https://issues.redhat.com/browse/KFLUXUI-796 from happening.
Type of change
Screen shots / Gifs for design review
Before (notice on-pull PLR status is
Running, and the test to check if the status is Cancelled succeeded):After (notice on-pull PLR status is
Running, and the test to check if the status is Cancelled correctly failed):How to test or reproduce?
Browser conformance:
Summary by CodeRabbit