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

frontend: Stabilization for e2e tests #1876

Merged

Conversation

gorankokin
Copy link
Contributor

@gorankokin gorankokin commented Apr 10, 2023

Stabilized e2e Cypress test for Data Pipelines project. No regression are expected, because only tests are changed and some attributes are added/changed in Components html templates.
Tested local build, linking built lib to UI and manual validation for different screen, e2e tests executed headless and headed and seems there is no problems. ESLint executed successfully and there no issues.

  • Changes includes mainly propagated e2e tests from SC which were stabilized there, under different initiative (issue).
  • With tests propagation there was need to add attributes on some HTML elements and minor changes in Components HTML templates, which shouldn't break anything and are validated manually and with e2e runs locally on my machine.
  • There are minor scripts changes and added one env variable test_environment in gitlab variable to adopt backpropagated stabilized tests from SC.
  • Improved sidebar menu navigation where Explore and Manage section are not expanded by default and works i sync with URL
  • Added root spinner that would show loading state while static files are getting fetch in Browser before application get bootstrapped
  • Changed Cypress default timeouts.

Issue: #1769

@gorankokin gorankokin added the area/frontend Related to changes in the folder projects/frontend for details about ui please see readme label Apr 10, 2023
@gorankokin gorankokin force-pushed the person/gorankokin/fix-stabilization-e2e-dp-tests-1769 branch from c6555ad to 7d6d5b0 Compare April 10, 2023 20:38
@DeltaMichael
Copy link
Contributor

Can you please add a short list of the changes you made? It's a bit hard to tell just from the diff.

@gorankokin gorankokin force-pushed the person/gorankokin/fix-stabilization-e2e-dp-tests-1769 branch 2 times, most recently from cf86653 to 6b57af2 Compare April 11, 2023 09:41
@gorankokin gorankokin force-pushed the person/gorankokin/fix-stabilization-e2e-dp-tests-1769 branch from 6b57af2 to e734017 Compare April 11, 2023 12:25
@gorankokin gorankokin force-pushed the person/gorankokin/fix-stabilization-e2e-dp-tests-1769 branch 2 times, most recently from 6d331df to a22d0d0 Compare April 11, 2023 13:14
Copy link
Contributor

@ivakoleva ivakoleva left a comment

Choose a reason for hiding this comment

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

The recommendation is to split those changes, into separate PRs.
With accordance with our guidelines https://github.com/vmware/versatile-data-kit/wiki/How-to-prepare-a-new-PR#break-your-code-commits-into--small-self-contained-units it is expected to take the necessary time, to have all the changes you may want. Otherwise, you may think you'll speedup multiple changes on the back of one single change needed - however that's a workaround, and shifts the responsibility to the Reviewers, to take enormous efforts trying to understand what all ~100 files changed are about.

@gorankokin gorankokin force-pushed the person/gorankokin/fix-stabilization-e2e-dp-tests-1769 branch from 47713a9 to 8a010c0 Compare April 11, 2023 13:32
@gorankokin
Copy link
Contributor Author

@ivakoleva I agree with you, but not totally. I don't agree that in this way responsibilities are shifted to reviewers because all of us are working in the best interests of the project and no one is trying to workaround something or push bad code, or not working code in production.
Everything is done in the best interests of the projects and to make easier for all members in the project to develop and contribute in a stable environment.

@ivakoleva
Copy link
Contributor

ivakoleva commented Apr 11, 2023

@ivakoleva I agree with you, but not totally. I don't agree that in this way responsibilities are shifted to reviewers because all of us are working in the best interests of the project and no one is trying to workaround something or push bad code, or not working code in production.
Everything is done in the best interests of the projects and to make easier for all members in the project to develop and contribute in a stable environment.

Hi @gorankokin, please split the critical change out of the cosmetic changes, to speed it up.
The recommendation is, upon each change, to create a PR describing why the change is needed, and what the change is, and how was it tested.

@ivakoleva ivakoleva removed the request for review from hzhristova April 11, 2023 14:39
@gorankokin gorankokin force-pushed the person/gorankokin/fix-stabilization-e2e-dp-tests-1769 branch from 8a010c0 to 6b95e32 Compare April 11, 2023 19:08
Copy link
Contributor

@ivakoleva ivakoleva left a comment

Choose a reason for hiding this comment

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

LGTM? May resolve the comments and merge?

@gorankokin
Copy link
Contributor Author

One thing that I need to mention is that with this PR e2e tests execution time is almost at the limit with ~53mins while the CI/CD job limit is 1h.
Maybe we will need to allocate a specific gitlab runner with increased resources (one more CPU), because execution artefacts (videos, images, hars) compression takes around 15mins.
With increased resources I believe we will go down to 40-45mins, because local execution finish in about 18mins.

@gorankokin gorankokin force-pushed the person/gorankokin/fix-stabilization-e2e-dp-tests-1769 branch from 6b95e32 to 8d80c62 Compare April 12, 2023 15:54
Copy link
Contributor

@ivakoleva ivakoleva 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 think we need to resolve the comments and merge 😊

@ivakoleva
Copy link
Contributor

One thing that I need to mention is that with this PR e2e tests execution time is almost at the limit with ~53mins while the CI/CD job limit is 1h.
Maybe we will need to allocate a specific gitlab runner with increased resources (one more CPU), because execution artefacts (videos, images, hars) compression takes around 15mins.
With increased resources I believe we will go down to 40-45mins, because local execution finish in about 18mins.

That's a good catch, may add a ticket with those observations, so we can prioritise it?

@ivakoleva ivakoleva removed their request for review April 19, 2023 07:11
Copy link
Contributor

@ivakoleva ivakoleva left a comment

Choose a reason for hiding this comment

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

Lets resolve the comments and merge?
Also, are the e2e tests stable lately?

@murphp15 murphp15 enabled auto-merge (squash) April 20, 2023 09:11
@gorankokin gorankokin force-pushed the person/gorankokin/fix-stabilization-e2e-dp-tests-1769 branch 2 times, most recently from 5ee2614 to f9dbbb1 Compare April 24, 2023 11:27
@gorankokin gorankokin force-pushed the person/gorankokin/fix-stabilization-e2e-dp-tests-1769 branch 10 times, most recently from 67e6775 to 33ac58e Compare April 25, 2023 12:14
@gorankokin gorankokin enabled auto-merge (squash) April 25, 2023 12:14
@gorankokin gorankokin force-pushed the person/gorankokin/fix-stabilization-e2e-dp-tests-1769 branch 2 times, most recently from 0cd6e67 to d5c21c7 Compare April 25, 2023 13:47
@gorankokin gorankokin enabled auto-merge (rebase) April 25, 2023 13:48
@gorankokin gorankokin force-pushed the person/gorankokin/fix-stabilization-e2e-dp-tests-1769 branch from f3c2619 to 931f008 Compare April 25, 2023 14:57
@gorankokin gorankokin force-pushed the person/gorankokin/fix-stabilization-e2e-dp-tests-1769 branch 2 times, most recently from 9fac011 to 625adf7 Compare April 25, 2023 18:04
* Stabilized e2e Cypress test for Data Pipelines project.
* No regression are expected, because only tests are changed
and some attributes are added/changed in Components
html templates.
* Madet tests atomic with dedicated Data Jobs, that are
created before suite and deleted after suite.
* Minor scripts changes, added one env variable
test_environment in gitlab variable to adopt backpropagated
stabilized tests from SC.
* Improved sidebar menu navigation where Explore and Manage
section are not expanded by default and works in URL sync.
* Added root spinner that would show loading state while
static files are fetched in Browser before application
get bootstrapped.
* Tested local build, linking built lib to UI and manual
validation for different screen, e2e tests executed
headless and headed and seems there is no problems.
* ESLint executed successfully and there no issues.

Signed-off-by: gorankokin <gorankokin@gmail.com>
@gorankokin gorankokin force-pushed the person/gorankokin/fix-stabilization-e2e-dp-tests-1769 branch from 625adf7 to 462efd1 Compare April 25, 2023 18:46
@gorankokin gorankokin merged commit 38921d6 into main Apr 25, 2023
6 checks passed
@gorankokin gorankokin deleted the person/gorankokin/fix-stabilization-e2e-dp-tests-1769 branch April 25, 2023 19:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/frontend Related to changes in the folder projects/frontend for details about ui please see readme cla-not-required
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants