Closed (fixed)
Project:
Experience Builder
Component:
Documentation
Priority:
Major
Category:
Plan
Assigned:
Unassigned
Issue tags:
Reporter:
Created:
5 Jun 2024 at 03:43 UTC
Updated:
1 Jul 2024 at 11:04 UTC
Jump to comment: Most recent
Problem/Motivation
In https://git.drupalcode.org/project/experience_builder/-/merge_requests/5 we added Cypress
It is being used for testing components and E2E tests (albeit via example tests).
We should set some heuristics regarding what to use where for tests.
The repo already includes Vitest and @testing-library - these are industry standards for testing components in isolation.
We also have MSW for mocking HTTP responses.
Steps to reproduce
Proposed resolution
- Only use Cypress for E2E tests (functional tests).
- Use Vitest and testing-library for component tests. With @testing-library/user-event we can also testing component interactions.
Remaining tasks
Agree
Setup CI to run existing Vitest tests
Fix broken Vitetest tests
Require component tests (bare minimum smoke/render test) for new components
User interface changes
API changes
Data model changes
Issue fork experience_builder-3452584
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
changes
Comments
Comment #2
larowlanComment #3
wim leersI know @bnjmnm and @jessebaker have discussed this and are aligned, but we should still capture that in docs.
Let's add document this in
ui/README.md.Comment #4
wim leersWe need to get clarity here to unblock #3452781: Add tests for undo/redo.
Comment #5
bnjmnmI have been happy with Cypress component testing and, similar to Cypress e2e, - my students have been able to be productive in it far more quickly than other component testing options, and were even able to independently figure out features like mocking and spies. The Cypress application works for component testing like it does for e2e which makes debugging significantly easier too. We've also been pleased with cypress-react-selector which lets us make
querySelector()esque queries but using component types.I'd like to use Cypress for as much as possible due to its approachability / ease of use, but I'll also mention that most of my Cypress component testing has been for fairly simple applications so it's possible there are advanced use cases that make it an unwise option.
I think there's a huge benefit in having a larger number of contributors able to write & understand tests even if it's at the expense of some technical benefits, but I'm also willing to accept when the tradeoffs no longer justify such a choice, so if there are specific concerns regarding Cypress + component that might not justify the ease-of-use benefits please share as I'm not a line-in-the-sand drawer.
Comment #6
larowlanThanks for that background Ben
My experience with Cypress has not been positive, I've come to associate it with brittle, slow and flaky test in client projects.
I think that has been down to how they have implemented it. I note you mention query selector and in my experience that has been the main issue. The tests have been written closely coupled to the Dom and in the end they've repeated a lot of the same mistakes people made with Behat.
I note that testing library has a set of eslint rules https://testing-library.com/docs/ecosystem-eslint-plugin-testing-library/ that explicitly deny interacting with the Dom via query selector and the like. Instead you're encouraged to write tests that make use of selectors that reflect what the user sees, eg
getByRole('button', {name: 'Submit'})and others like getByText.I've written some unit tests for reducers on the undo/redo test issue. I'll expand that to include a component test with testing library. I'll then write a second MR that uses Cypress for the same. We then will have two to compare.
I'm not opposed to using Cypress if that's the preference, but we should try to mimic best practices and avoid proliferation of query selectors in tests.
Comment #7
larowlanHere's some sample vitests that show unit tests for state, and some component tests - https://git.drupalcode.org/project/experience_builder/-/merge_requests/38
Here's a sample unit test for cypress - https://git.drupalcode.org/project/experience_builder/-/merge_requests/43 - unfortunately it doesn't run because we can't load components/files form outside tests/Cypress. I think we will need to merge the package.json in tests/Cypress with the one in ui in order to use it for component/unit testing. We can use it where it is for E2E.
Comment #8
larowlanGetting a cypress fail here https://git.drupalcode.org/issue/experience_builder-3452828/-/jobs/1800000 but it might be a general CI failure
Comment #9
ressaThis is great news! I asked about testing last year in the Drupal Forum, and @jaypan kindly made me aware of Cypress and how it works. I agree that it is very easy to set up, and create tests with it in Drupal. After trying it, me and @jaypan created Browser testing using Cypress.
Comment #10
jessebaker commentedAs @bnjmnm said above, we would like to keep as much of our testing in Cypress as possible (for both consistency and for ease of use reasons).
@bnjmnm is working hard to try and get the infrastructure in place to have the Cypress tests that @larowlan wrote run on the pipeline but it's taking longer than expected as he's being pulled in many different directions at the moment.
Tests being slow is perhaps something we will need to be aware of, and work to mitigate, as I have also seen that be a problem in the past. The other issues are absolutely something we can work around though. I don't believe Cypress is inherently brittle or flakey and I think we can/should absolutely put measures in place to ensure tests are written in such a way as to avoid that such as the eslint plugin mentioned.
I think using @testing-library/cypress is an absolute must to ensure that tests are as stable and non-flakey and maintainable as possible and I've used it extensively in the past to good effect.
Comment #11
wim leersTo resolve this, I think we need:
ui/README.mdThe first thing is in progress. The second should happen in a separate issue. The third is pure documentation, and can hence happen here.
Comment #12
wim leersMaking the previous comment more concrete:
Comment #14
wim leers@jessebaker + @larowlan are +1 to this — now up to @bnjmnm for sign-off and merging 😊
Comment #16
bnjmnm+1 from me as well. Maybe this will change a bit over time but this is the foundation to do it from.
Comment #17
wim leers