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.

Sign up for GitHub

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

Jump to bottom

fix(cypress): improve cypress performances #47379

Merged
merged 6 commits into from
Aug 22, 2024
Merged

fix(cypress): improve cypress performances #47379

merged 6 commits into from
Aug 22, 2024

Conversation

skjnldsv
Copy link
Member

@skjnldsv skjnldsv commented Aug 21, 2024 •
edited
Loading

  • 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, nfebe, 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 skjnldsv force-pushed the fix/cypress-stop-cache branch from 4165e9f to d690435 Compare August 21, 2024 15:34
@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 enabled auto-merge August 21, 2024 18:38
@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.

Sign in to view
@susnux

This comment was marked as resolved.

Sign in to view
@skjnldsv

This comment was marked as resolved.

Sign in to view
@skjnldsv skjnldsv force-pushed the fix/cypress-stop-cache branch from 708dc35 to 1ecbe72 Compare August 22, 2024 07:00
@skjnldsv

This comment was marked as resolved.

Sign in to view
@skjnldsv skjnldsv force-pushed the fix/cypress-stop-cache branch 2 times, most recently from 482035f to 91b8681 Compare August 22, 2024 10:17
@skjnldsv
fix(cypress): do not install deps twice
49fa2e5 
Signed-off-by: skjnldsv <skjnldsv@protonmail.com>
@skjnldsv
chore(cypress): allow db snapshot and restore fo faster tests
b6bc288 
Signed-off-by: skjnldsv <skjnldsv@protonmail.com>
@skjnldsv
chore(cypress): put nextcloud into ram
5a7e211 
Signed-off-by: skjnldsv <skjnldsv@protonmail.com>
@skjnldsv
chore(cypress): skip cypress binary install on init step
c0781f5 
Signed-off-by: skjnldsv <skjnldsv@protonmail.com>
@skjnldsv
chore(cypress): enable apcu
d3a0e83 
Signed-off-by: skjnldsv <skjnldsv@protonmail.com>
@skjnldsv
fix(cypress): drop unwanted docker output characters
5e2e2ba 
Signed-off-by: skjnldsv <skjnldsv@protonmail.com>
@skjnldsv skjnldsv force-pushed the fix/cypress-stop-cache branch from 91b8681 to 5e2e2ba Compare August 22, 2024 11:33
artonge
artonge reviewed Aug 22, 2024
View reviewed changes
.github/workflows/cypress.yml Show resolved Hide resolved
cypress/e2e/settings/personal-info.cy.ts
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 skjnldsv mentioned this pull request Aug 22, 2024
Merged
@skjnldsv
Copy link
Member Author

Follow-up, use functions from @nextcloud/cypress

@susnux
Copy link
Contributor

susnux commented Aug 22, 2024 •
edited
Loading

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?

susnux
susnux approved these changes Aug 22, 2024
View reviewed changes
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 👍

@skjnldsv skjnldsv mentioned this pull request Jan 7, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@artonge artonge artonge approved these changes

@susnux susnux susnux approved these changes

@nfebe nfebe Awaiting requested review from nfebe nfebe was automatically assigned from nextcloud/server-frontend

@sorbaugh sorbaugh Awaiting requested review from sorbaugh sorbaugh was automatically assigned from nextcloud/server-frontend

@Pytal Pytal Awaiting requested review from Pytal

Assignees

@skjnldsv skjnldsv

Labels
3. to review Waiting for reviews developer experience performance 🚀
Projects
None yet
Milestone
Nextcloud 31
Development

Successfully merging this pull request may close these issues.
4 participants
@skjnldsv @SystemKeeper @susnux @artonge