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

Add unit tests for useConstrainedTabbing #47427

Open
afercia opened this issue Jan 25, 2023 · 3 comments · May be fixed by #48736
Open

Add unit tests for useConstrainedTabbing #47427

afercia opened this issue Jan 25, 2023 · 3 comments · May be fixed by #48736
Labels
[Status] In Progress Tracking issues with work in progress [Type] Automated Testing Testing infrastructure changes impacting the execution of end-to-end (E2E) and/or unit tests. [Type] Enhancement A suggestion for improvement.

Comments

@afercia
Copy link
Contributor

afercia commented Jan 25, 2023

Description

Related: #47426
Related: #34836

#34836 refactored useConstrainedTabbing but missed to add unit test for the specific use case the refactoring was meant to cover, see #34681.
There should be also some test for the basic functionality of the hook.

It would be great to add a few tests, also noting that some of the other hooks do have tests. I tried in the context of #47426 but my knowledge is limited and I have no idea how to make the hook actually run in a test (it uses useRefEffecta internally). any help would be greatly appreciated.

Step-by-step reproduction instructions

See the hook doesn't have unit tests.

Screenshots, screen recording, code snippet

No response

Environment info

No response

Please confirm that you have searched existing issues in the repo.

Yes

Please confirm that you have tested with all plugins deactivated except Gutenberg.

Yes

@afercia afercia added [Type] Enhancement A suggestion for improvement. [Type] Automated Testing Testing infrastructure changes impacting the execution of end-to-end (E2E) and/or unit tests. labels Jan 25, 2023
@afercia afercia self-assigned this Mar 3, 2023
@afercia afercia linked a pull request Mar 3, 2023 that will close this issue
@github-actions github-actions bot added the [Status] In Progress Tracking issues with work in progress label Mar 3, 2023
@afercia afercia removed their assignment Dec 18, 2024
@afercia
Copy link
Contributor Author

afercia commented Dec 18, 2024

Removing myself as assignee for this issue. It's still a valid problem though.
As mentioned in the attempted PR #48736 I doubt the current implementation is testable because it relies on a native browser behavior that is hardly testable.

Overall, I'm not sure (to be fair) that any non-testable code should be merged in this project.
I'd appreciate any thoughts and feedback Cc @WordPress/gutenberg-core

@ciampo
Copy link
Contributor

ciampo commented Dec 19, 2024

One option is to use a real browser environment to run such tests (ie. Playwright) — the quickest way could be to add a suite of e2e tests.

@afercia
Copy link
Contributor Author

afercia commented Dec 20, 2024

@ciampo thabks for your feedback. E2e tests would test the browser behavior. The whole point here is that the current implementation relies on native browser behaviors that may unexpectedly differ and they did already led to bugs we had to fix. There have been a few with Safari for example.

I do realize it's time consuming but if you check the previous implementation before #34681 / #34836 it was more 'under control' and more testable. It was changed because of a specific case that could have been addressed in a different way, keeping the implementation under control. Instead, it now relies on browser behavior.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Status] In Progress Tracking issues with work in progress [Type] Automated Testing Testing infrastructure changes impacting the execution of end-to-end (E2E) and/or unit tests. [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants