Improve E2E setup

[yannikmesserli]

No issue

Description

This PR aims to simplify our E2E setup, so that it is easier for anyone to understand it (and hopefully faster for them to write tests) and easier to maintain.

Changes

• Moved all test selectors and attributes to an enum at top of component: this will prevent building "legacy". Components are responsible for maintaining their test ids and if the component disapears, so do the test ids related, breaking the E2E scenarios using it.
• Removed src/testing/e2e/cypress/client which were the main source of legacy building
• Added a utils for the E2e tests: getTestAttribute, which helps adding the common data-test (and avoid attributes to be shown in production)
• Moved up all cypress's folder
• Documented the various Cypress command

After this PR, E2E test are written as follow:

• Add an enum E2E on top of the component you'd like to interact with:

export enum E2E {
   myTestId = "test-id",
}

• Attach the test attribute(s) using getTestAttributes as follow in your component:

<div {...getTestAttributes(E2E.myTestId)} />

or:

<div {...getTestAttributes({ [E2E.myTestAttr]: "some value" })} />

• Use Cypress's new command query or queryAttrs to get the component in your scenario, importing the E2E test id within your test:

import { E2E } from '~/components/my-component';
// ...
cy.query(E2E.myTestId);
// or
cy.queryAttrs({ [E2E.myTestAttr]: "some value" });

Screenshots

Doc for queryAttrs in VSCode	Doc for query in VSCode

Unit tests

Not added. We could add some for getTestAttributes...

Functional tests

The e2e tests should still run and pass

Future work

• We need to document the setup for e2e and how to write e2e test scenarios
• The repo https://bitbucket.org/yandzee/hubble-ui-testing.git with test data needs to somehow be brought back in this repo (or at least in a cilium repo)

[yandzee]

I like changes regarding doc lines and webpack configuration in cypress setup, but that it because all other changes are making things worse.

We already had a conversation about this kind of changes, so I don't really understand why you still want to have them. I cannot approve this PR since such changes merely violates many simple architecture principles.

• You suggest to use stateless getTestAttributes function right in UI components as a replacement for method call of the entity who is responsible to control the attributes for any logical part of component. This can be considered as a step backward from abstracting over that entity to concrete implementation.
• With that you have to pass a special identifier into that function guiding e2e code from this end. This is kind a similar violation as what Cypress docs refer to when saying you not to rely to native html/css of components so that you could establish proper elements querying: the generalization of this principle could sound like - its not the component who should decide what attributes should be put for e2e tests. I would say this is also a step backward to depending on implementation rather then abstraction
• As a consequences of this changes now you suggest to make e2e module dependable on the UI components itself - you must import those defined attributes from somewhere. This is also not correct: e2e module should define what they should use for tests, not vice versa.
• A classical indicator of such architecture violation is that since you no more depend on abstraction/higher level responsible entity you have to customize all other details around used hardcoded implementation - this is what we see here in AccessPoint and ServiceMapArrowDuckFeet components - they are starting to define its own helpers with strong coupling on tests details. You will have to define its own helpers in each component in such an architecture.

[yandzee]

You are going to depend on each module/component that is being tested, right?

[yannikmesserli]

Exactly the goal of this PR.

[yandzee]

👍

[yannikmesserli]

Summary of discussion with @yandzee on 26 Feb 2024:

• We couldn't agree on the architecture (yet)
• @yandzee wants the frontend to not know anything about the tests. Instead, creating the logic for the various keys inside the test folder, and the components to call them.
• @yannikmesserli wants the frontend to have the test keys within the component and the tests importing them, so that we don't have to maintain logic for tests, simplify the setup.
• We decided that both will seek examples of cypress setup in other projects.

Feel free to chip in to help us decide which route we should go.

[yandzee]

That's true in general, minor comments:

yandzee wants the frontend to not know anything about the tests. Instead, creating the logic for the various keys inside the test folder, and the components to call them.

Sure its better to minimize the amount of test logic you blend into UI logic/components. I don't want to have keys, I want to have abstract methods working in the way like "hey, here is the FlowsTable column, you may put some attributes for it if you want". In contrast what Yannik suggestion: "hey, I am the FlowsTable and its me who decides what attributes I am going to have, where and when"

[yannikmesserli]

This has been rejected by @yandzee and we could not find consensus within our team of developer. I am therefore closing it.

[rolinh]

We discussed this PR internally and agreed that this patch successfully addresses the goals of simplifying our e2e test setup and will make them easier to maintain in the long run. Although we may not all agree on some specifics of this PR, we agree that it is a net improvement overall and we should thus work on getting it merged.