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

Sap system details e2e #529

Merged
merged 5 commits into from May 17, 2022
Merged

Sap system details e2e #529

merged 5 commits into from May 17, 2022

Conversation

arbulu89
Copy link
Contributor

Add E2E tests for the SAP system details view, ported from the old trento legacy project.

Besides that, I have taken advantage and unify the links to the host details view using the new component HostLink

@arbulu89 arbulu89 marked this pull request as ready for review May 13, 2022 13:46
Copy link
Member

@nelsonkopliku nelsonkopliku left a comment

Choose a reason for hiding this comment

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

Looks good! Just a little leftover about page refreshing.

assets/js/components/HostLink.jsx Show resolved Hide resolved
});
/* This test is commented because there is not any option to remove added SAP instances or
resetting the database afterwards, and it affects the rest of the test suite.
it(`should show a new instance when an event with a new SAP instance is received`, () => {
Copy link
Member

Choose a reason for hiding this comment

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

This test would be about showing a newly discovered instance on the current sap system? Right?

Adding an extra fixture wouldn't work because it would affect all other tests, correct?

I believe this is calling for a more refined way to interact with the state in e2e test.

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 test actually works, and the functionality too, but as future tests are only based in the original 27 nodes landscape, and we are not able to remove the recently added instance, everything else starts failing.
Here there are 2 solutions:

  • We implement the option to reset the database and start in a clean env
  • We implement the SAP instances de-registration (this is actually a feature that is already in our backlog)

Copy link
Member

Choose a reason for hiding this comment

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

Implementation of the SAP instance de-registration is, as you mention, something on the backlog we'll be working on.
So I'd keep it out from this discussion.

If I am understanding properly, the problem is that when we load the scenario sap-system-detail-NEW - or any scenario - we change the state and other tests would be negatively impacted by this.

cy.loadScenario(`sap-system-detail-NEW`);

By having the resetDatabase command in cypress we'd be able to clean up the state and restart.
I am afraid this might turn out not that straightforward because elixir complains about playing around with the database it is connected to (dropping, recreating databases/tables)

There might be other strategies playing with also photofinish but I am not sure right now.
We might want to further discuss this.

I believe since we are working on the e2e debt, it would be beneficial to also take into account this activity.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nelsonkopliku Ticket for this reset database topic created in our team backlog as tech-debt

Copy link
Member

@nelsonkopliku nelsonkopliku left a comment

Choose a reason for hiding this comment

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

LGTM.

I would track the outcome of this discussion #529 (comment)

@arbulu89 arbulu89 merged commit b080e47 into main May 17, 2022
@arbulu89 arbulu89 deleted the sap-system-details-e2e branch May 17, 2022 06:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci enhancement New feature or request
Development

Successfully merging this pull request may close these issues.

None yet

2 participants