Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add end-to-end deregistration tests for Host Details page #1649

Merged
merged 5 commits into from
Jul 20, 2023

Conversation

jamie-suse
Copy link
Contributor

Description

Adds end-to-end deregistration tests for the Host Details page.

How was this tested?

By adding end-to-end deregistration tests for the Host Details page.

@jamie-suse jamie-suse added javascript Pull requests that update Javascript code test labels Jul 19, 2023
@@ -355,4 +355,41 @@ context('Host Details', () => {
cy.get('span').find('svg').should('exist');
});
});

describe('Deregistration', () => {
describe('"Clean up" button should be visible only for an unhealthy host', () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar tests for the visibility of the Clean up button are also already tested in the React component tests (see HostDetails.test.jsx) Do you think they are necessary in end-to-end too?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, let's have them!

});

it('should allow to deregister a host after clean-up confirmation', () => {
cy.contains('button', 'Clean up', { timeout: 15000 }).click();
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The value of timeout depends on Elixir environment variable config :trento, deregistration_debounce. These timeouts are also present in the Hosts List tests for the same reason. Could we abstract this to an environment variable somehow?

Copy link
Contributor

@arbulu89 arbulu89 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jamie-suse I see that we use a totally different way compared with the HostList view. I think we should be consistent, and after the other is merged, follow the same logic.
@rtorrero Could you assist on this? And review as you have the best insights!

@@ -355,4 +355,41 @@ context('Host Details', () => {
cy.get('span').find('svg').should('exist');
});
});

describe('Deregistration', () => {
describe('"Clean up" button should be visible only for an unhealthy host', () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, let's have them!

describe('"Clean up" button should be visible only for an unhealthy host', () => {
it('should not display the "Clean up" button for healthy host', () => {
cy.task('startAgentHeartbeat', [selectedHost.id]);
cy.contains('button', 'Clean up').should('not.exist');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! Actually I prefer this method over the one @rtorrero implemented in the host list view hehe
But to keep the same, we should still use other way around. Ruben added a data-testid so, let's use it I guess.
What do you think @rtorrero ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually preferred this one too, but when I tried it for the confirmation dialog (which also has the text Clean up) it would get only the button from the table and not the one in the modal. If you find a way for this to work in all cases, this would be preferred but otherwise I'd keep the other approach.

cy.get('@modal').contains('button', 'Clean up').click();

cy.get('@modal').should('not.exist');
cy.url().should('eq', cy.config().baseUrl + '/hosts');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should check that the row in the host list view for this host goes away

@jamie-suse jamie-suse force-pushed the host-details-dereg-e2e branch from e0ac49f to 101ff08 Compare July 20, 2023 08:31
@jamie-suse jamie-suse requested a review from arbulu89 July 20, 2023 12:21
it('should allow to deregister a host after clean-up confirmation', () => {
cy.contains('button', 'Clean up', { timeout: 15000 }).click();

cy.get('#headlessui-portal-root').as('modal');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regarding my previous comment: I think this is how you solved the contains getting the Clean up button from the modal. If that's so, then I'm up for it. I'll create a tech debt to update the other deregistration e2e tests.

Copy link
Contributor

@rtorrero rtorrero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM now @jamie-suse , thanks!

@jamie-suse jamie-suse merged commit 678132c into main Jul 20, 2023
@jamie-suse jamie-suse deleted the host-details-dereg-e2e branch July 20, 2023 14:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
javascript Pull requests that update Javascript code test
Development

Successfully merging this pull request may close these issues.

3 participants