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

Use Cypress Testing Library - composer.spec.ts #10511

Merged
merged 2 commits into from
Apr 5, 2023
Merged

Use Cypress Testing Library - composer.spec.ts #10511

merged 2 commits into from
Apr 5, 2023

Conversation

luixxiul
Copy link
Contributor

@luixxiul luixxiul commented Apr 4, 2023

This PR applies custom functions on find.ts based on Cypress Testing Library to composer.spec.ts test to improve E2E test flow. Please note that since Rich Text Editor (RTE) does not have the ARIA role yet, findTextbox() cannot be applied to it.

type: task

For element-hq/element-web#25033

Signed-off-by: Suguru Hirahara luixxiul@users.noreply.github.com

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.

Signed-off-by: Suguru Hirahara <luixxiul@users.noreply.github.com>
@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 Apr 4, 2023
@luixxiul luixxiul marked this pull request as ready for review April 4, 2023 15:44
@luixxiul luixxiul requested a review from a team as a code owner April 4, 2023 15:44
@luixxiul luixxiul requested review from dbkr and richvdh April 4, 2023 15:44
@richvdh
Copy link
Member

richvdh commented Apr 4, 2023

Sorry, I'm a bit confused about what this PR is seeking to change.

  1. The subject is "Use Cypress Testing Library..." - but surely that would be a much bigger change than this? Is this just laying some groundwork for a future change? It would be good to clarify the subject if so.
  2. The description says "... findTextbox() is not available for the composer yet", but the PR appears to be trying to use this method which is apparently not available. Isn't this a contradiction?

@luixxiul
Copy link
Contributor Author

luixxiul commented Apr 5, 2023

Good morning @richvdh ,

  1. The subject is "Use Cypress Testing Library..." - but surely that would be a much bigger change than this? Is this just laying some groundwork for a future change? It would be good to clarify the subject if so.

The library was implemented recently by #10446 as you would know, and I guessed that while using it where possible should be a preferable solution for E2E tests, the official employees would not have time to replace the existing code which works just fine, so I thought I could take the task. I am wondering if you would think that an issue for tracking the task for applying the library to the Cypress E2E tests should be created. -> It was created here: element-hq/element-web#25033

  1. The description says "... findTextbox() is not available for the composer yet", but the PR appears to be trying to use this method which is apparently not available. Isn't this a contradiction?

I have forgot to mention, but findTextbox() is going to be applied only inside CIDER, as cy.get("div[contenteditable=true]") inside "Rich text editor" cannot be replaced with findTextbox() yet. Because WYSIWYG composer RTE is under active development and I thought changing it (maybe by adding ARIA textbox role) should be deferred to another PR, this PR does not change cy.get("div[contenteditable=true]") inside "Rich text editor". (I am going to create an issue that Rich Text Editor currently lacks the ARIA role, which should be added to improve a11y.)

I hope that the above would answer to your questions. Thanks,

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 for the explanation. Looks good to me!

@richvdh richvdh merged commit 78e03e0 into matrix-org:develop Apr 5, 2023
@luixxiul luixxiul deleted the test-composer branch April 5, 2023 10:51
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.

2 participants