Add test to check for Clean up button visibility #1629

rtorrero · 2023-07-13T10:06:48Z

Description

This PR simply adds:

e2e test to check if the "Clean up" button in the hosts overview is only displayed after a failed heartbeat
e2e test to check if the "Clean up" button displays a confirmation and removes the host after confirmation

arbulu89

@rtorrero I would go with a new file for the deregistration (deregistration.cy.js). This topic is so big, and involves more things that a unique file would be better.

Besides this, I miss the fact that we test that the button is there when the heartbeat fails.
I would prefer to have something like

Heartbeat sending - button is nothere
Heartbeat stopped - button shows up
Heartbeat sending again - button goes away

arbulu89 · 2023-07-13T12:30:29Z

test/e2e/cypress/e2e/hosts_overview.cy.js

@@ -170,4 +170,42 @@ context('Hosts Overview', () => {
      });
    });
  });
+
+  describe('Clean-up', () => {


Maybe Deregistration?

arbulu89 · 2023-07-13T12:33:49Z

test/e2e/cypress/e2e/hosts_overview.cy.js

+
+      it('should hide all Clean-up buttons when heartbeat starts except the first one', () => {
+        cy.get(
+          ':nth-child(1) > .w-48 > .bg-white > .text-jungle-green-500'


What about this: tr:nth-child(1) > td:nth-child(9) where the 1st 1 is the row number.
The other query selector looks totally confusing

arbulu89 · 2023-07-13T12:37:58Z

test/e2e/cypress/e2e/hosts_overview.cy.js

+      });
+
+      it('should remove a host after confirming host cleanup', () => {
+        cy.get('.bg-jungle-green-500').click();


This test depends in the previous one. The test should be executable by itself, so it should set the host in deregisterable state, open the modal, and everything else.

arbulu89 · 2023-07-13T12:38:33Z

test/e2e/cypress/e2e/hosts_overview.cy.js

+        );
+      });
+
+      it('should remove a host after confirming host cleanup', () => {


Cannot we get the button by name of something? The class usage looks weak and really not meaningful

rtorrero · 2023-07-13T13:40:26Z

Hey @arbulu89, thanks for the quick review :-)

@rtorrero I would go with a new file for the deregistration (deregistration.cy.js). This topic is so big, and involves more things that a unique file would be better.

I'm not sure about this: up until now, we have organized all the tests by section / view:

about
checks_catalog
clusters_overview
... etc

I'd find it confusing to mix these and now introduce splitting by "feature"

Also, organizing the test files by section / view reduces the total run time of the e2e test suite as it minimizes transitioning from one part of trento to a different one (the deregistration / cleanup feature spans through multiple views). We would check the cluster details view for the cluster details specific tests and then we'd need to navigate again to it while running the deregistration tests)

I'd instead use http calls to our backend in the views that are affected by the deregistration (e.g. cluster details) to deregister a single host and, while I'd use the button in those views that have it.

Besides this, I miss the fact that we test that the button is there when the heartbeat fails. I would prefer to have something like
* Heartbeat sending - button is nothere

* Heartbeat stopped - button shows up

* Heartbeat sending again - button goes away

I was indeed doing this but as you said, depending on the previous test. I'll improve this and to it explicitly as you suggest, being able to run tests independently is indeed important.

arbulu89

Some additional comments from my side

arbulu89 · 2023-07-17T08:14:20Z

assets/js/components/DeregistrationModal/DeregistrationModal.jsx

@@ -30,7 +30,12 @@ function DeregistrationModal({
          onClick={onCleanUp}
        >
          <EOS_CLEANING_SERVICES size="base" className="fill-white inline" />
-          <span className="text-white text-sm font-bold pl-1.5">Clean up</span>
+          <span
+            id="cleanup-confirm"


This is just for testing?
You should maybe use data-testid i guess

I know the click event will bubble up to the button element, so it still works, but isn't it more proper to place this ID on the button?

In any case, the ID for testing might not even be required, in previous tests we selected the confirm clean up button like this:

screen.getAllByRole('button', { name: 'Clean up' })[1]

web/assets/js/components/HostsList.test.jsx

Line 173 in 73f91dc

const cleanUpModalButton = screen.getAllByRole('button', {

@jamie-suse That seems from react's testing library, not sure if I can use it in cypress. In any case, based on the official docs https://docs.cypress.io/guides/references/best-practices, data-cy seems to be the preferred way.

edit: or data-test or data-testid

arbulu89 · 2023-07-17T08:22:03Z

test/e2e/cypress/e2e/hosts_overview.cy.js

+        cy.task('startAgentHeartbeat', heartbeatTargets);
+      });
+
+      it('should hide all Clean-up buttons when heartbeat starts except the first one', () => {


I personally still don't like this testing approach.
I don't really think we should use so many agents. I would prefer to use one/two agent, and test thing more thorougly.
As commented, a thing that I would do is:

Send heartbeat for this agent. Check button is not there

Stop sending hearbeat. Check the button shows up. This is the most important test. I know that it takes time (10 seconds) to reach the heartbeat failed scenario, but otherwise we are not testing this explicitily

Send hearbeat again and check button goes away

arbulu89 · 2023-07-17T08:24:58Z

test/e2e/cypress/e2e/hosts_overview.cy.js

+      const heartbeatTargets = agents();
+      const failingHost = heartbeatTargets.shift();
+      before(() => {
+        cy.task('startAgentHeartbeat', heartbeatTargets);


Shouldn't we navigate to the hosts view here?
I know that we do in the main beforeAll, but if we don't do it here, we are already basing our tests in the order of things. For example, before this describe we have the healthDetection tests, and these tests would already be affected by those.
What do you think?

Good point @arbulu89 I guess that if any of the previous tests would do a cy.visit(...) it could break. I'll add a

cy.visit('/hosts'); cy.url().should('include', '/hosts');

Maybe for a future PR we could change the top before() for a beforeEach() so that this URL visit/check is performed for all tests without having to add it explicitly

test/e2e/cypress/e2e/hosts_overview.cy.js

arbulu89

Looks good @rtorrero
I wouldn't add the photofinish new feature if it is not used in the tests.
I understand that you added because you were using it locally for your development

arbulu89 · 2023-07-19T06:51:05Z

test/e2e/cypress/e2e/hosts_overview.cy.js

@@ -170,4 +170,64 @@ context('Hosts Overview', () => {
      });
    });
  });
+
+  describe('Deregistration', () => {
+    const host = {


Maybe we could call this hostToDeregister. But not a big deal

arbulu89 · 2023-07-19T06:52:03Z

.photofinish.toml

@@ -202,3 +202,13 @@ files = [
 files = [
    "./test/fixtures/scenarios/host-details/9cd46919-5f19-59aa-993e-cf3736c71053_cloud_discovery_unknown.json",
 ]
+
+[vmdrbddev01]


This is not used anywhere.
I would avoid adding things that you only use locally for your tests.
If you need in reallity, just add in the corresponding PR

Add test to check for Clean up button visibility

167b30a

rtorrero added javascript Pull requests that update Javascript code test labels Jul 13, 2023

Add test to check for 'Clean up' button logic

8219025

rtorrero requested review from arbulu89, CDimonaco, jamie-suse and dottorblaster July 13, 2023 12:01

rtorrero marked this pull request as ready for review July 13, 2023 12:02

arbulu89 reviewed Jul 13, 2023

View reviewed changes

Address style comments

04dd027

rtorrero mentioned this pull request Jul 14, 2023

E2e test deregistration cascading #1634

Merged

arbulu89 reviewed Jul 17, 2023

View reviewed changes

jamie-suse reviewed Jul 17, 2023

View reviewed changes

test/e2e/cypress/e2e/hosts_overview.cy.js Outdated Show resolved Hide resolved

rtorrero requested review from jamie-suse and arbulu89 July 18, 2023 12:53

jamie-suse approved these changes Jul 18, 2023

View reviewed changes

arbulu89 approved these changes Jul 19, 2023

View reviewed changes

rtorrero force-pushed the add-e2e-cleanup-button-test branch from 828a186 to a2521c5 Compare July 19, 2023 07:34

rtorrero added 3 commits July 19, 2023 08:35

Use data-testid for cypress targeting

4c28e34

Make deregistration testing more robust

e9324a7

Rename host to hostToDeregister

a2521c5

rtorrero merged commit da55221 into main Jul 19, 2023

rtorrero deleted the add-e2e-cleanup-button-test branch July 19, 2023 08:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add test to check for Clean up button visibility #1629

Add test to check for Clean up button visibility #1629

rtorrero commented Jul 13, 2023 •

edited

Loading

arbulu89 left a comment

arbulu89 Jul 13, 2023

arbulu89 Jul 13, 2023

arbulu89 Jul 13, 2023

arbulu89 Jul 13, 2023

rtorrero commented Jul 13, 2023

arbulu89 left a comment

arbulu89 Jul 17, 2023

jamie-suse Jul 17, 2023

rtorrero Jul 17, 2023 •

edited

Loading

arbulu89 Jul 17, 2023

arbulu89 Jul 17, 2023

rtorrero Jul 17, 2023

arbulu89 left a comment

arbulu89 Jul 19, 2023

arbulu89 Jul 19, 2023

Add test to check for Clean up button visibility #1629

Add test to check for Clean up button visibility #1629

Conversation

rtorrero commented Jul 13, 2023 • edited Loading

Description

arbulu89 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rtorrero commented Jul 13, 2023

arbulu89 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rtorrero Jul 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arbulu89 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rtorrero commented Jul 13, 2023 •

edited

Loading

rtorrero Jul 17, 2023 •

edited

Loading