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

Restore actual browser testing with Cypress #289

Closed
stefcameron opened this issue Apr 7, 2021 · 8 comments · Fixed by #291
Closed

Restore actual browser testing with Cypress #289

stefcameron opened this issue Apr 7, 2021 · 8 comments · Fixed by #291

Comments

@stefcameron
Copy link
Member

We removed Karma in #226 but that removed browser-based testing. tabbable, focus-trap, and focus-trap-react now all use the @testing-library and focus-trap/react use Cypress. Let's add Cypress tests to tabbable.

@stefcameron
Copy link
Member Author

Fixing this would also help fix #50

@idoros
Copy link
Contributor

idoros commented Apr 9, 2021

I have this working locally, I really wanted to have a comparable real list of tabbable elements extracted from the browser by automatically setting a fixture between 2 tabbable elements and tabbing between them, but Cypress tab functionality is not there yet. maybe in the future.

I don't really see any reason to keep 2 types of tests (or keep both jest and mocha/chai), can I just move the current tests to Cypress?

@stefcameron
Copy link
Member Author

but Cypress tab functionality is not there yet. maybe in the future.

But over in focus-trap, we tab between elements: https://github.com/focus-trap/focus-trap/blob/master/cypress/integration/focus-trap-demo.spec.js#L65

Can that not be used with what you're trying to test? The link you pointed to looks more like @testing-library/user-event's userEvent.type('{tab}') vs userEvent.tab(), but even though userEvent.tab() actually works surprisingly well, even in JSDom, we're talking about Cypress, which seems to have the ability to tab between elements just fine AFAICT.

I don't really see any reason to keep 2 types of tests (or keep both jest and mocha/chai), can I just move the current tests to Cypress?

I don't think we need to duplicate tests between testing-library/Jest and Cypress. I'm fine with migrating all the existing integration tests (which is probably most of them) to Cypress. But I'd like to keep testing-library/Jest setup to use for unit tests since that work has already been done. I'd prefer not to use Cypress to test for things like error conditions in API misuse, for example, even if that's in the future. By having it in place, it'll be easier to ask for a unit test to cover something in a PR than to also ask to do all the TL/Jest setup (again) in it too.

Makes sense?

@idoros
Copy link
Contributor

idoros commented Apr 11, 2021

But over in focus-trap, we tab between elements

I will test it out, if it works I will commit it in a separate commit under the same PR

...migrating all the existing integration tests (which is probably most of them)

I think the line is thin here, and all test should move, I wouldn't keep the jest tests just for a couple of tests (not sure which one you would keep). Anyway I will keep it running if you insist, but I will remove one of the CI jobs, as there is definitely no need to test on both node 12 and 14, especially as this code will never run on either in production.

Another thing that I'm not sure about is code coverage merge between Cypress and Jest. if you have some experience or an example to point to then it will help.

@idoros
Copy link
Contributor

idoros commented Apr 12, 2021

But over in focus-trap, we tab between elements

Looked at what's being done there and it doesn't really do what I want. You use cypress-plugin-tab, which uses ally.js ability to search for tabbable elements (in a similar way that tabbable works) and then set focus to the next tabbable element.

I want to validate against the actual browser tab order, validating with another library code will just keep tabbable as good as the other library and might even be wrong.

@idoros idoros mentioned this issue Apr 12, 2021
9 tasks
@stefcameron
Copy link
Member Author

cypress-io/cypress#299 is requesting adding tab() support in Cypress, since 2016. Well, that's a miss on my part as well.

I agree our Cypress tests won't be better than what ally defines until Cypress has native tab support, which could still be a long time.

Dare I say, then, we should revert back to Karma for tabbable, assuming Karma with its headless Chrome and FireFox browser plugins, was actually placing the code in a real browser? The only additional tests added since we replaced those tests with jest/testing-library were the ones you created for the new displayCheck option you added just last week, so the revert shouldn't be that hard (famous last words...).

But if we do this, how will you "press the tab key" anyway? The previous Karma setup, AFAIK, won't give you a way to trigger the tab sequence and follow through it. But it will probably give you the DOM APIs you were wanting for displayCheck.

@idoros
Copy link
Contributor

idoros commented Apr 13, 2021

Although they seem to work on real events, I also have doubts about the time that it would take and it seems their solution doesn't work in firefox :/ also I find it odd that their implementation isn't async.

Looking forward, to get user interaction api we need something like Playwright, the downside of Playwright/Puppeteer is that they run their tests over on the node side, making tests slow (every interaction with the page is async) + they don't deal with building your code for the browser. I have a solution that I use over at zeejs, which bundles the code, opens Playwright, runs the test in the browser and collect the results + gives access to Playwright api through a bridge between node and browser, so tests running in the browser can run fast while still having access to the api (mouse hover, tab, image snapshot). I plan on moving this solution and publishing it, but I can't recommend this as a solution at the moment because there is still work to mature it.

My solution is based on play-mocha that does the same thing without the api bridge. It's also hard for me to recommend as it's not a known solution, but I know the developers behind it and trust them.

To summarize, at the moment Cypress doesn't help us with real user events, but it lets us test in a real browser, and as you said Karma wouldn't help us with that either, so I don't see the difference. As much as I would like to have browser validation for test expectations, I think moving to browser testing is a good first step that will allow us to start web components support, and we can always move later.

@stefcameron
Copy link
Member Author

Sounds good. Cypress stays, then, and we go forward from there. 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants