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

fix(cypress): improve cypress performances #47379

Merged
merged 6 commits into from
Aug 22, 2024
Merged

Conversation

skjnldsv
Copy link
Member

@skjnldsv skjnldsv commented Aug 21, 2024

  • Prevent fetching all deps again with npm ci for each runner (node_modules is already saved in the init step)
  • Enable redis to speed things up
  • Allow restoring DB snapshots and implement it o the profile spec (personal-info.cy.ts)
  • Make Nextcloud setup into RAM
Before After
2024-08-21_20-37 2024-08-21_20-17

@skjnldsv skjnldsv force-pushed the fix/cypress-stop-cache branch 4 times, most recently from e29bedc to 4165e9f Compare August 21, 2024 15:32
@skjnldsv skjnldsv self-assigned this Aug 21, 2024
@skjnldsv skjnldsv requested review from a team, artonge, Fenn-CS, sorbaugh, susnux and Pytal and removed request for a team August 21, 2024 15:32
@skjnldsv skjnldsv added this to the Nextcloud 31 milestone Aug 21, 2024
@skjnldsv skjnldsv marked this pull request as ready for review August 21, 2024 15:33
@skjnldsv
Copy link
Member Author

Also tested a new approach, restroring DB snapshots.
A good example is the profile test going from 3 minutes, 22 seconds to 2 minutes, 36 seconds

@skjnldsv skjnldsv force-pushed the fix/cypress-stop-cache branch 2 times, most recently from aa8856e to 64ae622 Compare August 21, 2024 17:33
@skjnldsv skjnldsv changed the title fix(cypress): do not install deps twice fix(cypress): improve cypress performances Aug 21, 2024
@skjnldsv skjnldsv force-pushed the fix/cypress-stop-cache branch 3 times, most recently from 3b48ff3 to 708dc35 Compare August 21, 2024 20:48
@SystemKeeper

This comment was marked as resolved.

@susnux

This comment was marked as resolved.

@skjnldsv

This comment was marked as resolved.

@skjnldsv

This comment was marked as resolved.

@skjnldsv skjnldsv force-pushed the fix/cypress-stop-cache branch 2 times, most recently from 482035f to 91b8681 Compare August 22, 2024 10:17
Signed-off-by: skjnldsv <skjnldsv@protonmail.com>
Signed-off-by: skjnldsv <skjnldsv@protonmail.com>
Signed-off-by: skjnldsv <skjnldsv@protonmail.com>
Signed-off-by: skjnldsv <skjnldsv@protonmail.com>
Signed-off-by: skjnldsv <skjnldsv@protonmail.com>
Signed-off-by: skjnldsv <skjnldsv@protonmail.com>
.github/workflows/cypress.yml Show resolved Hide resolved
cy.modifyUser(user, 'locale', 'en_US')
})

cy.wait(500)
Copy link
Contributor

Choose a reason for hiding this comment

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

Seem arbitrary. Can't we wait for something more specific?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, we'll standardise this a bit more in a followup.

Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this needed?
As soon as this part is reached the create user request has terminated so all database actions are already finished as the PHP process is done.
So normally this should not be needed I guess?

@skjnldsv
Copy link
Member Author

Follow-up, use functions from @nextcloud/cypress

@susnux
Copy link
Contributor

susnux commented Aug 22, 2024

This is much better, thank you!
But there is still a performance issue, see runner 3:

Overall Run: 13 minutes 14 seconds
Cypress tests : 6 minutes 22 seconds
Pulling image: 1 minute
Restore context: 3 minutes

more than 50% of the CI time is still not related to tests.

Pulling the image takes locally for me much less time, I assume our runners do not have it cached and thus need to externally fetch it from GH?

But the thing that is weird: Creating the context takes ~2:30 while restoring takes ~3 minutes.
This are 27 minutes wasted by restoring the context, maybe this is something we can improve in a follow up?

Copy link
Contributor

@susnux susnux left a comment

Choose a reason for hiding this comment

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

🚀

@skjnldsv skjnldsv merged commit 4eebf47 into master Aug 22, 2024
173 checks passed
@skjnldsv skjnldsv deleted the fix/cypress-stop-cache branch August 22, 2024 12:32
@skjnldsv
Copy link
Member Author

But the thing that is weird: Creating the context takes ~2:30 while restoring takes ~3 minutes.
This are 27 minutes wasted by restoring the context, maybe this is something we can improve in a follow up?

It's decompressing the archive I think, it takes many IO.

more than 50% of the CI time is still not related to tests.

That would be my guess as well, the more we have running, the more load our CI servers have. Meaning it will obviously have an impact on other runs.

@skjnldsv
Copy link
Member Author

Pulling the image takes locally for me much less time, I assume our runners do not have it cached and thus need to externally fetch it from GH?

It does, we have a docker layer cache. We store images.
But this one is a new image, and we have 4 CI servers. Maybe that was a cache miss occurence, should be better now 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants