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

E2e restoration #1665

Merged
merged 23 commits into from
Jul 27, 2023
Merged

E2e restoration #1665

merged 23 commits into from
Jul 27, 2023

Conversation

rtorrero
Copy link
Contributor

Description

This PR adds e2e tests for the restoration of hosts after they have been deregistered. It checks that the right details are restored and visible in:

  • hosts overview
  • sap systems overview
  • clusters overview
  • dbs overview
  • cluster details
  • sap system details
  • hana db details

Notice that to do this, we have added additional photofinish scenarios that reuse the existing discovery json messages.

@rtorrero rtorrero marked this pull request as ready for review July 24, 2023 14:20
Copy link
Contributor

@jamie-suse jamie-suse left a comment

Choose a reason for hiding this comment

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

This might be overkill, but what do you think about first asserting that the host/database/cluster/SAP system does not exist, then running the scenario and asserting that it has been restored?
I guess these assertions for nonexistence will take extra time to run though. Just a thought.

.photofinish.toml Outdated Show resolved Hide resolved
.photofinish.toml Outdated Show resolved Hide resolved
test/e2e/cypress/e2e/clusters_overview.cy.js Outdated Show resolved Hide resolved
@@ -18,4 +18,19 @@ context('Databases Overview', () => {
cy.contains(hdqDatabase.sid).should('not.exist');
});
});

describe('Restoration', () => {
const databaseToRestore = {
Copy link
Contributor

Choose a reason for hiding this comment

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

We have seen some recent failure on Databases and SAP systems restoration. The instances belonging to them were not recovered properly after a browser refresh.
Maybe we should consider those events.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added the test and indeed it fails. Oddly enough I can't consistently reproduce the bug manually, but cypress catches it.

test/e2e/cypress/e2e/clusters_overview.cy.js Outdated Show resolved Hide resolved
@rtorrero rtorrero requested a review from arbulu89 July 26, 2023 12:12
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.

@rtorrero much better!
Some last nitpicks.
If you want, we can leave the "remaining app instances are there after restoration" e2e test for the PR which includes the fix

test/e2e/cypress/e2e/clusters_overview.cy.js Show resolved Hide resolved
test/e2e/cypress/e2e/databases_overview.cy.js Outdated Show resolved Hide resolved
test/e2e/cypress/e2e/hana_cluster_details.cy.js Outdated Show resolved Hide resolved
test/e2e/cypress/e2e/hosts_overview.cy.js Outdated Show resolved Hide resolved
test/e2e/cypress/e2e/hosts_overview.cy.js Outdated Show resolved Hide resolved
@rtorrero rtorrero requested a review from arbulu89 July 26, 2023 15:10
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.

Still some last things @rtorrero
Anyway, just approving the PR, we spent already too much time on it 😅
Let's wait until the fix is there to see if the test passes and we can move forward

test/e2e/cypress/e2e/databases_overview.cy.js Show resolved Hide resolved
test/e2e/cypress/e2e/databases_overview.cy.js Outdated Show resolved Hide resolved
id: '240f96b1-8d26-53b7-9e99-ffb0f2e735bf',
name: 'vmhdbdev01',
id: '13e8c25c-3180-5a9a-95c8-51ec38e50cfc',
tablePos: 7,
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks so ugly 😅
I would rename it to tableRow anyway

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I improved it a bit by removing the dependency on the table position, should look better

@rtorrero rtorrero requested a review from jamie-suse July 27, 2023 09:17
@rtorrero rtorrero merged commit 7a449b6 into main Jul 27, 2023
@rtorrero rtorrero deleted the e2e-restoration branch July 27, 2023 17:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants