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

Deactivate Materialize animations #22291

Draft
wants to merge 1 commit into
base: 5.x-dev
Choose a base branch
from
Draft

Conversation

mneudert
Copy link
Member

@mneudert mneudert commented Jun 6, 2024

Description:

A lot of UI tests are using timeouts to wait for animations to complete, with varying degrees of success and unnecessary runtime increase.

There are already some places that try to deactivate animations for UI tests:

Both CSS and jQuery should work as expected, but the Materialize does not. Probably because M.anime is not accessed from the global namespace, but passed directly to the components.

This is an attempt to change the override to work until we can for example upgrade to Materialize 2.

@matomo-org/core-reviewers I tried two CI runs to check the benefit/problems with this solution:

  • CI run with lots of page.waitForTimeout removed
  • CI run with the modification applied

To remove the statements I used sed -i '/waitForTimeout.*animation/d' **/*_spec.js, so I would not expect it to catch all the right places or even the most impactful ones.

Refs DEV-18077

Review

@mneudert mneudert added c: Tests & QA For issues related to automated tests or making it easier to QA & test issues. RFC Indicates the issue is a request for comments where the author is looking for feedback. Technical debt Issues the will help to reduce technical debt labels Jun 6, 2024
@mneudert mneudert self-assigned this Jun 6, 2024
@michalkleiner
Copy link
Contributor

Seems with the timeouts removed there are screenshots in the middle of the animation. With the new override there's a few screenshots that look better and a few that look worse, so I guess we'd need to look at them one by one. In general I think this is a good approach and a change that will help speed up the UI test runs.

Copy link
Contributor

If you don't want this PR to be closed automatically in 28 days then you need to assign the label 'Do not close'.

@github-actions github-actions bot added the Stale The label used by the Close Stale Issues action label Jun 21, 2024
Copy link
Contributor

github-actions bot commented Aug 2, 2024

This PR was last updated more than one month ago, maybe it's time to close it. Please check if there is anything we still can do or close this PR. ping @matomo-org/core-reviewers

@github-actions github-actions bot added the Stale for long The label used by the Close Stale Issues action label Aug 2, 2024
@michalkleiner michalkleiner added Do not close PRs with this label won't be marked as stale by the Close Stale Issues action and removed Stale The label used by the Close Stale Issues action Stale for long The label used by the Close Stale Issues action labels Aug 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: Tests & QA For issues related to automated tests or making it easier to QA & test issues. Do not close PRs with this label won't be marked as stale by the Close Stale Issues action RFC Indicates the issue is a request for comments where the author is looking for feedback. Technical debt Issues the will help to reduce technical debt
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants