Skip to content
This repository has been archived by the owner on Sep 11, 2024. It is now read-only.

Add E2E test of audio player #10441

Merged
merged 75 commits into from
Apr 6, 2023
Merged

Add E2E test of audio player #10441

merged 75 commits into from
Apr 6, 2023

Conversation

luixxiul
Copy link
Contributor

@luixxiul luixxiul commented Mar 24, 2023

This PR adds E2E test of audio player.

For element-hq/element-web#24747 and element-hq/element-web#22542

This test includes taking Percy snapshots to check if the player is rendered properly on various scenarios (on light theme, dark theme, high contrast theme, with a reply, a reply chain, IRC layout, modern/group layout, bubble layout).

I would like to ask a reviewer to add X-Needs-Percy label so that I could check if the Percy tests should work as intended and fix the issue before this PR is merged and the test becomes a flaky one on develop branch (I implemented several ideas which hopefully should prevent the test from becoming flaky).

The Percy screenshots should be captured as below (captured manually on Cypress). Please note that the seek bar is hidden on Percy due to the flaky test reported here. Please note there is a difference between a local Cypress run and a Percy snapshot about how the list style is applied (element-hq/element-web#24969)

Screenshot
IRC layout Screenshot from 2023-03-24 20-09-37
Modern layout Screenshot from 2023-03-24 20-10-05
Bubble layout Screenshot from 2023-03-24 20-10-33
Monospace
(Modern layout)
Screenshot from 2023-03-24 20-11-05
Monospace
(bubble layout)
Screenshot from 2023-03-24 20-11-34
Dark theme
(modern layout)
Screenshot from 2023-03-24 20-12-03
Dark theme
(bubble layout)
Screenshot from 2023-03-24 20-12-37
High contrast
(modern layout)
Broken: reported here
Screenshot from 2023-03-24 20-13-19
High contrast
(bubble layout)
Broken: reported here
Screenshot from 2023-03-24 20-13-50
Reply
(modern layout)
Screenshot from 2023-03-24 20-15-00
Reply
(bubble layout)
Screenshot from 2023-03-24 20-15-28
Reply chain
(modern layout)
Screenshot from 2023-03-24 20-16-17
Reply chain
(bubble layout)
Screenshot from 2023-03-24 20-16-46

type: task

Retry of #10407

Signed-off-by: Suguru Hirahara [email protected]

Checklist

  • Tests written for new code (and old code if feasible)
  • Linter and other CI checks pass
  • Sign-off given on the changes (see CONTRIBUTING.md)

This change is marked as an internal change (Task), so will not be included in the changelog.

- Add a long name audio file to check file name overflow
- Check player layout on monospace font
- Check player layout on high contrast theme
- Check player layout on IRC layout (should be same as modern layout)
- Percy snapshots of mx_MAudioBody (hide seekbar temporarily)
- Output snapshot logs

Signed-off-by: Suguru Hirahara <[email protected]>
Signed-off-by: Suguru Hirahara <[email protected]>
Signed-off-by: Suguru Hirahara <[email protected]>
Signed-off-by: Suguru Hirahara <[email protected]>
Signed-off-by: Suguru Hirahara <[email protected]>
@github-actions github-actions bot added Z-Community-PR Issue is solved by a community member's PR T-Task Refactoring, enabling or disabling functionality, other engineering tasks labels Mar 24, 2023
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Signed-off-by: Suguru Hirahara <[email protected]>
Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

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

Thanks very much for this, @luixxiul, it looks like a great addition.

I've started taking a look but I have a few questions.

cypress/e2e/audio-player/audio-player.spec.ts Outdated Show resolved Hide resolved
cypress/e2e/audio-player/audio-player.spec.ts Outdated Show resolved Hide resolved
cypress/e2e/audio-player/audio-player.spec.ts Outdated Show resolved Hide resolved
cypress/e2e/audio-player/audio-player.spec.ts Outdated Show resolved Hide resolved
cypress/e2e/audio-player/audio-player.spec.ts Outdated Show resolved Hide resolved
cypress/e2e/audio-player/audio-player.spec.ts Outdated Show resolved Hide resolved
cypress/e2e/audio-player/audio-player.spec.ts Outdated Show resolved Hide resolved
cypress/e2e/audio-player/audio-player.spec.ts Outdated Show resolved Hide resolved
cypress/e2e/audio-player/audio-player.spec.ts Outdated Show resolved Hide resolved
cypress/e2e/audio-player/audio-player.spec.ts Outdated Show resolved Hide resolved
@luixxiul

This comment was marked as outdated.

Signed-off-by: Suguru Hirahara <[email protected]>
@luixxiul luixxiul requested a review from richvdh April 5, 2023 04:23
Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

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

I think this looks good, but I'd like to see what Percy does with it. Trying to find out whether we can make Percy run on this PR...

cypress/e2e/audio-player/audio-player.spec.ts Show resolved Hide resolved
@luixxiul

This comment was marked as outdated.

Signed-off-by: Suguru Hirahara <[email protected]>
@richvdh
Copy link
Member

richvdh commented Apr 5, 2023

Updated the branch to (hopefully) have Percy take snapshots.

Apparently we've run out of Percy quota, and it won't be restored until the 21st April, so I think we'll have to just go ahead and merge this

Signed-off-by: Suguru Hirahara <[email protected]>
@luixxiul
Copy link
Contributor Author

luixxiul commented Apr 5, 2023

@richvdh it looks auto merge will not happen until the quota is restored. Should we wait?

@richvdh
Copy link
Member

richvdh commented Apr 5, 2023

@richvdh it looks auto merge will not happen until the quota is restored. Should we wait?

@t3chguy is going to sort this out

@t3chguy
Copy link
Member

t3chguy commented Apr 5, 2023

@luixxiul I will land this once the merge queue unblocks itself - #10515

@t3chguy t3chguy removed the X-Needs-Percy Whether to run Percy screenshot tests in Merge Queue label Apr 5, 2023
@t3chguy t3chguy disabled auto-merge April 5, 2023 17:15
@t3chguy
Copy link
Member

t3chguy commented Apr 5, 2023

@luixxiul can you pull latest develop please

@t3chguy t3chguy enabled auto-merge (squash) April 6, 2023 08:44
@t3chguy t3chguy merged commit 374a23e into matrix-org:develop Apr 6, 2023
@luixxiul luixxiul deleted the test-audioplayer branch April 6, 2023 09:17
@luixxiul luixxiul restored the test-audioplayer branch April 6, 2023 09:39
@luixxiul
Copy link
Contributor Author

I checked the latest snapshots and they have been successfully taken: https://percy.io/dfde73bd/matrix-react-sdk/builds/26747313/search?searchParam=audio%20player

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
T-Task Refactoring, enabling or disabling functionality, other engineering tasks Z-Community-PR Issue is solved by a community member's PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants