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: introduce cypress e2e tests #9520

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

perzeuss
Copy link
Contributor

@perzeuss perzeuss commented Oct 20, 2024

Checklist:

  • Please open an issue before creating a PR or link to an existing issue
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I ran dev/reformat(backend) and cd web && npx lint-staged(frontend) to appease the lint gods

Description

As described in #9253, this PR introduces cypress e2e tests to test Dify features. The setup is using gherkin & cucumber to define test cases. This makes it easier to write tests in a more human-readable format and also acts as documentation for the features.

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update, included: Dify Document
  • Improvement, including but not limited to code refactoring, performance optimization, and UI/UX improvement
  • Dependency upgrade

Testing Instructions

Run docker test environment an execute Cypress tests

  • Go to the /web directory and run yarn to install the dependencies
  • Run yarn test:e2e:ci to run the cypress tests
    • This will run a Dify API and required databases configured in docker/docker-compose.cypress.yaml. Data is stored in a separate volume
    • This will start the frontend devserver
    • Cypress tests are only executed after both the api and frontend are up and running

The tests should pass and you should see the recorded videos in web/cypress/videos. Since the tests run fast, the videos will only be helpful if a test fails. If you want to generate videos that can be used as reference for docs, run yarn test:e2e:docs which will generate tests that are slower and allow to follow to what happens in the frontend.

Opening the Cypress UI

For the development of tests, you would want to open the Cypress UI.

  • Run yarn cy:run, an electron window should open where you can select the e2e test suite and run individual tests
  • If your dev environment runs on a remote machine without GUI, you can visit http://localhost:6080/ to see the GUI via VNC. If you are on windows with wsl, you can run yarn cy:open:wsl and use VcXsrv to connect to the VNC server and interact with the Cypress UI.

GitHub Action

I've added a GitHub action to run the tests. For a performance boost, it will create and use a docker image as cache. For this we need to configure the secret CYPRESS_CONTAINER_REGISTRY_TOKEN (classic token with registry:write permission for docker push) and the variables DOCKER_NAMESPACE and DEVCONTAINER_IMAGE_NAME in the repository settings.

You can find a test run of the GitHub action here: https://github.com/perzeuss/dify/actions/runs/11428886478

@dosubot dosubot bot added size:XL This PR changes 500-999 lines, ignoring generated files. 📚 documentation Improvements or additions to documentation labels Oct 20, 2024
@perzeuss perzeuss changed the title feat: introduce cypress e2e integration tests feat: introduce cypress e2e tests Oct 20, 2024
@perzeuss perzeuss force-pushed the feat/9253-introduce-cypress-e2e-integration-tests branch from 9f8c95d to b19a88c Compare October 23, 2024 23:21
@perzeuss perzeuss force-pushed the feat/9253-introduce-cypress-e2e-integration-tests branch from b19a88c to eba2e7d Compare October 23, 2024 23:22
@crazywoola crazywoola requested a review from iamjoel October 29, 2024 06:58
@crazywoola
Copy link
Member

@iamjoel Please take a look at this.

@iamjoel iamjoel requested a review from AkaraChen October 30, 2024 02:34
@iamjoel
Copy link
Collaborator

iamjoel commented Oct 30, 2024

@AkaraChen Please take a look at this.

@AkaraChen
Copy link
Member

@perzeuss maybe remove data-test properties in production will be better.

https://nextjs.org/docs/architecture/nextjs-compiler#remove-react-properties

# && apt-get -y install --no-install-recommends <your-package-list-here>

# Cypress dependencies - Cypress is our end-to-end testing tool
Copy link
Member

Choose a reason for hiding this comment

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

@takatost The frontend team rarely use devcontainer, and backend team don't care about e2e test, maybe remove these change will be better?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for your feedback! I understand the concerns about adding Cypress dependencies to the Dockerfile for the devcontainer. However, In my opinion, we have enough reasons to keep them:

  • We have frontend developers in the open-source community who may want to use the devcontainer for their contributions.
  • The devcontainer serves as the base image for running the e2e tests in the CI pipeline.
  • Even though the backend team may not prioritize end-to-end tests, it's essential for them to run these tests when making significant changes.
  • Installing these dependencies doesn't take up much space or time, and since the devcontainer is rarely rebuilt, the impact on development workflows is negligible.
  • Removing these dependencies adds another hurdle for contributors when it comes to running tests after making changes, which goes against the purpose of devcontainers. They are meant to provide a seamless development environment that just works, making it easier for everyone to contribute effectively.

@perzeuss
Copy link
Contributor Author

@perzeuss maybe remove data-test properties in production will be better.

https://nextjs.org/docs/architecture/nextjs-compiler#remove-react-properties

I wouldn't recommend removing testids in production unless they cause problems. Keeping them in the production build can even be beneficial: they can assist in reporting bugs by helping to locate specific components, and they can also be used to run tests in the production environment. Apart from that, enabling a feature that uses regex to remove code comes with the risk that it will break things.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📚 documentation Improvements or additions to documentation size:XL This PR changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants