Remove flakiness from ssh-keys integration tests #7427

KhizerRehan · 2025-06-13T12:55:32Z

What this PR does / why we need it:

This PR fixes flakiness in ssh-keys integration tests

Which issue(s) this PR fixes:

Fixes #6940

What type of PR is this?

/kind bug
/find failing-test

Special notes for your reviewer:

Does this PR introduce a user-facing change? Then add your Release Note here:

NONE

Documentation:

NONE

kubermatic-bot · 2025-06-13T12:55:36Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

KhizerRehan · 2025-06-13T12:56:49Z

/test all

KhizerRehan · 2025-06-13T14:48:14Z

/test pre-dashboard-web-check

KhizerRehan · 2025-06-16T13:04:22Z

/test all

KhizerRehan · 2025-06-16T13:15:55Z

/test pre-dashboard-web-check

ahmadhamzh

small comments

ahmadhamzh · 2025-06-17T07:15:24Z

modules/web/cypress/types/condition.ts

@@ -26,6 +26,7 @@ export enum Condition {
  NotBe = 'not.be',
  NotBeChecked = 'not.be.checked',
  NotBeVisible = 'not.be.visible',
+  NotBeDisabled = 'not.be.disabled',


isn't NotBeDisabled is the same of BeEnabled

“not disabled” == “enabled” Another way of testing assertions using negative expression but if that make harder form understanding POV let me know wdyt?

personally, I think it might be a bit redundant, but I’m okay if you feel it makes more sense to keep it.

ahmadhamzh · 2025-06-17T07:15:32Z

modules/web/cypress/pages/v2/projects/page.ts

-    this.Buttons.openDialog.click();
-    this.Elements.addDialogInput.type(name).then(_ => this._strategy?.onCreate());
+    this.Buttons.openDialog.click({force: true});
+    this.Elements.addDialogInput.should(Condition.BeVisible).should(Condition.NotBeDisabled);


shouldn't we use Condition.BeEnabled instead

Makes no difference IMO, if we use Not.Be.Disabled or Be.Enabled which technically means input should ready for input keystrokes

Reasoning same as above “not disabled” == “enabled” but if "Be.Enabled" makes more sense let me know.

Happy to Change!

thx

ahmadhamzh · 2025-06-17T07:16:29Z

modules/web/cypress/fixtures/routes.json

+{
+  "/api/v1/*": "/$1",
+  "/api/v2/*": "/$1"
+}


we need to add an empty line at the end of the file here

also why do we need this file ?
we have one inside fixtures/routes.json ?

IMO, there is only routes.json / db.json (checking in main branch)

Under this path

Currently, there’s only routes.json and db.json in main.

In this PR, I have moved them into cypress/fixtures for better organization — they’re related to e2e tests.

From my understanding, they’re mainly used to make sure json-server runs and aren’t used for test mocks.

We already have datacenter(s).json files and Endpoints.ts for API endpoints.

FYI: Instead of deleting i have moved these files into cypress/fixtures folder + updated package.json script to PICK those files from cypress/fixtures path

let me know if something problematic in the following change or my understanding

Thx

sounds good 👍
but i think we need to add an empty line at the end of the file here

ahmadhamzh · 2025-06-17T07:25:56Z

modules/web/cypress/e2e/v2/stories/ssh-keys.spec.ts

+      if ($warning.length > 0) {
+        cy.reload();
+      }
+    });


should we add this to settings/defaults.spec.ts ?

We can but TBH, i didn't found an issue while running default.spec however we can add it but I think there is not need to add extra check for default.spec unless it was problematic same as of ssh-keys.spec

thoughts?

one other thing is that if '.no-data-warning' is not found then it will fail right ? so i think that is not right

@ahmadhamzh did you mean .no-data-warning if class is removed or changed to something else it shouldn't fail or did you mean to keep trying multiple times and fail then?

Ah! I see what you mean in case warning doesn't exists cypress will try to fetch warning and if it fails test will fail too. Which shouldn't be the case. Hmmm I see.

ahmadhamzh · 2025-06-17T07:29:22Z

modules/web/cypress/e2e/v2/stories/ssh-keys.spec.ts

@@ -59,8 +59,16 @@ describe('SSH Key Management Story', () => {
    Pages.expect(View.Clusters.Default);
  });

-  it('should create the cluster with ssh key', {retries: 3}, () => {
+  it('should create the cluster with ssh key', () => {
+    Pages.Clusters.List.visit();


isn't this already been done in the previous step ?

Each it block runs independently if it was handled in beforeEach block then we might not need this line but to be explicit we need to navigate to list page once this test runs.

thoughts?

IMO there's no point in visiting the clusters list page if we're not going to perform any action there.

Nevermind, I thought the Pages.Wizard.visit() is navigating directly to wizard. But in other case, @ahmadhamzh is correct, we're already navigating to clusters list page in previous test. I agree that all tests should run independently but in this case every test is dependent on previous one.

KhizerRehan

Feedback Response

KhizerRehan · 2025-06-17T07:40:34Z

modules/web/cypress/e2e/v2/stories/ssh-keys.spec.ts

@@ -59,8 +59,16 @@ describe('SSH Key Management Story', () => {
    Pages.expect(View.Clusters.Default);
  });

-  it('should create the cluster with ssh key', {retries: 3}, () => {
+  it('should create the cluster with ssh key', () => {
+    Pages.Clusters.List.visit();


Each it block runs independently if it was handled in beforeEach block then we might not need this line but to be explicit we need to navigate to list page once this test runs.

thoughts?

KhizerRehan · 2025-06-17T07:40:40Z

modules/web/cypress/e2e/v2/stories/ssh-keys.spec.ts

+      if ($warning.length > 0) {
+        cy.reload();
+      }
+    });


We can but TBH, i didn't found an issue while running default.spec however we can add it but I think there is not need to add extra check for default.spec unless it was problematic same as of ssh-keys.spec

thoughts?

KhizerRehan · 2025-06-17T07:44:22Z

modules/web/cypress/pages/v2/projects/page.ts

-    this.Buttons.openDialog.click();
-    this.Elements.addDialogInput.type(name).then(_ => this._strategy?.onCreate());
+    this.Buttons.openDialog.click({force: true});
+    this.Elements.addDialogInput.should(Condition.BeVisible).should(Condition.NotBeDisabled);


Makes no difference IMO, if we use Not.Be.Disabled or Be.Enabled which technically means input should ready for input keystrokes

Reasoning same as above “not disabled” == “enabled” but if "Be.Enabled" makes more sense let me know.

Happy to Change!

thx

KhizerRehan · 2025-06-17T07:46:10Z

modules/web/cypress/types/condition.ts

@@ -26,6 +26,7 @@ export enum Condition {
  NotBe = 'not.be',
  NotBeChecked = 'not.be.checked',
  NotBeVisible = 'not.be.visible',
+  NotBeDisabled = 'not.be.disabled',


“not disabled” == “enabled” Another way of testing assertions using negative expression but if that make harder form understanding POV let me know wdyt?

KhizerRehan · 2025-06-17T07:54:46Z

modules/web/cypress/fixtures/routes.json

+{
+  "/api/v1/*": "/$1",
+  "/api/v2/*": "/$1"
+}


IMO, there is only routes.json / db.json (checking in main branch)

Under this path

Currently, there’s only routes.json and db.json in main.

In this PR, I have moved them into cypress/fixtures for better organization — they’re related to e2e tests.

From my understanding, they’re mainly used to make sure json-server runs and aren’t used for test mocks.

We already have datacenter(s).json files and Endpoints.ts for API endpoints.

FYI: Instead of deleting i have moved these files into cypress/fixtures folder + updated package.json script to PICK those files from cypress/fixtures path

let me know if something problematic in the following change or my understanding

Thx

KhizerRehan · 2025-06-17T07:56:09Z

modules/web/fixtures/OWNERS

-# See the OWNERS docs: https://git.k8s.io/community/contributors/guide/owners.md
-
-approvers:
-  - sig-ui
-
-reviewers:
-  - sig-ui
-
-labels:
-  - sig-ui
-
-options:
-  no_parent_owners: true


Delete this files due to moving files under cypress/fixtures folder?

Any thoughts on this?

ahmadhamzh · 2025-06-17T09:11:24Z

modules/web/cypress/e2e/v2/stories/ssh-keys.spec.ts

+      if ($warning.length > 0) {
+        cy.reload();
+      }
+    });


one other thing is that if '.no-data-warning' is not found then it will fail right ? so i think that is not right

ahmadhamzh · 2025-06-17T09:42:54Z

modules/web/fixtures/OWNERS

are you sure we don't need this file anymore ?

Yes —
db.json and routes.json have been moved to modules/web/cypress/fixtures and related scripts updated in package.json.
Therefore, there’s no need for an OWNER file there anymore, as the directory no longer contains any files.

ahmadhamzh · 2025-06-17T09:52:40Z

modules/web/cypress/types/condition.ts

@@ -26,6 +26,7 @@ export enum Condition {
  NotBe = 'not.be',
  NotBeChecked = 'not.be.checked',
  NotBeVisible = 'not.be.visible',
+  NotBeDisabled = 'not.be.disabled',


personally, I think it might be a bit redundant, but I’m okay if you feel it makes more sense to keep it.

ahmadhamzh · 2025-06-17T10:15:29Z

modules/web/cypress/fixtures/routes.json

+{
+  "/api/v1/*": "/$1",
+  "/api/v2/*": "/$1"
+}


sounds good 👍
but i think we need to add an empty line at the end of the file here

KhizerRehan · 2025-06-17T10:42:08Z

modules/web/cypress/e2e/v2/stories/ssh-keys.spec.ts

+    Pages.Members.accessSideNavItem();
    Pages.SSHKeys.visit();


Test was failing as DDN menu was not opened and DOM node is not visible therefore fails.

Where as Pages.SSHKeys.visit() only clicks the button if DDN is opened.

reference test: should create the ssh key

…ad of class

…izard for cluster creation instead of dep on prev test case

kubermatic-bot · 2025-06-17T12:04:04Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign ahmadhamzh for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

KhizerRehan · 2025-06-17T14:01:24Z

/test pre-dashboard-web-integration-tests

…pec before cluster creation

KhizerRehan · 2025-06-17T15:53:14Z

/test pre-dashboard-web-integration-tests

KhizerRehan · 2025-06-17T16:02:33Z

/test pre-dashboard-web-integration-tests-ce

Waseem826 · 2025-06-17T19:18:24Z

/test pre-dashboard-web-integration-tests-ce

There was a failure (on different scenario) for ssh keys spec: https://minio.worker.ci.k8c.io/dashboard-cypress-artifacts/pr-7427-pre-dashboard-web-integration-tests-1934999113099644928/videos/ssh-keys.spec.ts.mp4

KhizerRehan added 2 commits June 13, 2025 16:38

Move Fixture inside cypress/fixture folder

e4756b5

Add Wait Fix test to access sidenav before accessing ssh keys pags

ab0f6a3

kubermatic-bot added do-not-merge/code-freeze Indicates that a PR should not merge because it has not been approved for code freeze yet. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 13, 2025

KhizerRehan added 3 commits June 16, 2025 15:53

Remove comment on reload

e5c792b

Fix to reload page when warning not exists

821d999

Add Existence assertion before button click

7fd6cc6

KhizerRehan marked this pull request as ready for review June 16, 2025 13:27

Fix Linting Issue

82c83c0

ahmadhamzh reviewed Jun 17, 2025

View reviewed changes

KhizerRehan commented Jun 17, 2025

View reviewed changes

ahmadhamzh reviewed Jun 17, 2025

View reviewed changes

KhizerRehan commented Jun 17, 2025

View reviewed changes

Fix EOF line

1a4bc7b

KhizerRehan added 2 commits June 17, 2025 17:02

Handle case in case warning not exist and use Id to get warning inste…

8617606

…ad of class

Add Safety seed warning check and explicit handles visit of cluster w…

5994ccb

…izard for cluster creation instead of dep on prev test case

KhizerRehan added 2 commits June 17, 2025 17:13

TS lint fix

3efea23

Handle Async nature and run commands after warning is checked

a09412c

kubermatic-bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 17, 2025

KhizerRehan added 2 commits June 17, 2025 19:49

Add Inline Conditional reload if warning exists in default/ssh-keys s…

11ae839

…pec before cluster creation

TS Lint fix

1bd47b3

kubermatic deleted a comment from kubermatic-bot Jun 17, 2025

Add Explicit wait and Assertion to validate body exists

d89ab84

Remove flakiness from ssh-keys integration tests #7427

Are you sure you want to change the base?

Remove flakiness from ssh-keys integration tests #7427

Conversation

KhizerRehan commented Jun 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kubermatic-bot commented Jun 13, 2025

Uh oh!

KhizerRehan commented Jun 13, 2025

Uh oh!

KhizerRehan commented Jun 13, 2025

Uh oh!

KhizerRehan commented Jun 16, 2025

Uh oh!

KhizerRehan commented Jun 16, 2025

Uh oh!

ahmadhamzh left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KhizerRehan Jun 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KhizerRehan Jun 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KhizerRehan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KhizerRehan Jun 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

KhizerRehan commented Jun 13, 2025 •

edited

Loading

KhizerRehan Jun 17, 2025 •

edited

Loading

KhizerRehan Jun 17, 2025 •

edited

Loading

KhizerRehan Jun 17, 2025 •

edited

Loading