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

feat(parallax): add e2e tests #1542

Merged
merged 16 commits into from
Jul 20, 2021
Merged

Conversation

kindoflew
Copy link
Collaborator

@kindoflew kindoflew commented Jun 4, 2021

Why

Draft of adding tests to eventually close #1474.

What

This adds several things:

  • A new demo app with vite, similar to the sandboxes
  • cypress and @testing-library/cypress
  • the actual tests -- parallax.spec.js

And also package.json scripts for dev and test. Everything new is inside packages/parallax.

Checklist

  • Documentation updated: n/a
  • Demo added: sort of, but n/a
  • Ready to be merged

Notes

CI

cypress requires a dev server to run tests. This can be manually done with two terminals open -- one for dev and one for cypress. But there's also start-server-and-test which would allow us to add it to the release pipeline (or at the very least, make things easier for contributors). I don't know how important that is, but it might be a nice-to-have? (Related -- should I add a script to package.json in the root folder to be able to run these tests from there as well?)

Actual tests

Currently, there are two main tests -- one each for a vertical and horizontal parallax app. In trying to make them actually E2E they just scroll/click and then check that everything is where it should be (by comparing their transform: translate3d values to what I know they should be, etc). But, after discovering the bug that lead to #1530 it seems that might not suffice (i.e. I think these tests might have passed even without that bugfix). Because of that I think it might be a good idea to add snapshot tests as well (but I still think testing the translate values is beneficial). I just wanted to get some general input before adding/removing more stuff.

Let me know if I'm totally off-base with anything, this is my first time attempting E2E tests (or really anything besides unit tests 😅).

@kindoflew kindoflew marked this pull request as draft June 4, 2021 18:00
@codesandbox-ci
Copy link

codesandbox-ci bot commented Jun 4, 2021

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 8285e3a:

Sandbox Source
spring-card Configuration
spring-goo-blobs Configuration
spring-flip-card Configuration
spring-slide Configuration
spring-draggable-list Configuration
spring-cards-stack Configuration
spring-viewpager Configuration
spring-simple-transition Configuration
spring-image-fade Configuration
spring-list-reordering Configuration
spring-masonry Configuration
spring-animating-auto Configuration
spring-multistage-transition Configuration
spring-chain Configuration
spring-svg-filter Configuration
spring-css-keyframes Configuration
spring-tree Configuration
spring-notification-hub Configuration
initial-rocket Configuration
spring-notification-hub Configuration
spring-notification-hub Configuration

@kindoflew
Copy link
Collaborator Author

kindoflew commented Jun 21, 2021

I was researching which screenshot plugin to use and I came across cypress-io/cypress#9443. This could make for flaky tests because Parallax has resize listeners that will cause animated changes to the layers' position and size.

For now, we can just stick with the direction we have and keep an eye on that issue and implement screenshots when it's fixed.

@joshuaellis
Copy link
Member

Hey, Sorry I missed the above comments in the main entry from your edits. Responses below:

should I add a script to package.json in the root folder to be able to run these tests from there as well

I think that would be good, we could edit yarn test to be yarn test:unit or something like that, and yarn test should run everything.

I think your test approach is a good idea, the translate value is the numerical source of truth here, but appreciate it might not always be accurate the dom being fickle. So adding snapshots sounds like a great idea. We should also make sure we add variation to the horizontal and vertical, to test things like sticky etc. But sounds good.

Good catch on the issue in cypress I think we should see how it goes and play from there.

@kindoflew
Copy link
Collaborator Author

I've added snapshots and a script to the root package.json. Each test (vertical and horizontal) has a normally translating layer (parent default horizontal), an opposite translating layer (if parent has horizontal={true}, then layer has horizontal={false}), a sticky layer, and a button for scrolling to the second page with scrollTo.

Each describe function has two it functions -- one for 'should translate layers as expected' which compares DOM snapshots and asserts translate values after scrolling. And one for 'should scroll to correct page with scrollTo'. The Cypress docs suggested that it's best practice to have large it functions, as opposed to a lot of little ones, so that's the structure I went with.

Are there any other variations you can think of that would be nice to test for?

@kindoflew kindoflew marked this pull request as ready for review July 9, 2021 18:04
@joshuaellis joshuaellis self-assigned this Jul 13, 2021
Copy link
Member

@joshuaellis joshuaellis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests look great! Just some thoughts around file structure that i've had, what do you think?

package.json Outdated Show resolved Hide resolved
packages/parallax/cypress/support/commands.js Outdated Show resolved Hide resolved
packages/parallax/package.json Outdated Show resolved Hide resolved
packages/parallax/demo/index.html Outdated Show resolved Hide resolved
Copy link
Member

@joshuaellis joshuaellis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Genuinely 👏🏼 amazing 👏🏼 work 👏🏼 thank you so much.

@joshuaellis joshuaellis merged commit b8d732e into pmndrs:master Jul 20, 2021
@kindoflew
Copy link
Collaborator Author

thanks! and no problem!

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 this pull request may close these issues.

[tests] E2E tests for Parallax
2 participants