-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
feat(surveys): Add cypress e2e tests #17920
Conversation
📸 UI snapshots have been updated1 snapshot changes in total. 0 added, 1 modified, 0 deleted:
Triggered by this commit. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! How broad do we generally want to be with these? Seems we could test even more things here, but ofc it's a tradeoff.
indeed, generally I don't want to test the UI looks perfectly correct, since the snapshots should take care of that. It's more about the functionality: clicking buttons works, and does the things you'd expect. But at the same time, don't want to get too deep into clicking every button in a drop down, that's just overkill, and don't want things to get annoying where every small change requires a cypress test update. I will write a few more when the UI polish is done, but this seemed like a reasonable basic functionality loop, which covers critical parts like when you add some targeting, it should stick around when you launch 🙈 (yes this was an old bug) |
Problem
~~Will go in after Eli's change: #17902 ~~
Changed my mind, these should be stable enough to work with/without the changes, and should help us catch issues. Happy to fix any tests that break with follow up changes.
But wanted to hit the ball running with e2e tests since things should be relatively stable after the overhaul
Changes
👉 Stay up-to-date with PostHog coding conventions for a smoother review.
How did you test this code?