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: 🎸 add end-to-end test harness #253

Merged
merged 2 commits into from
Aug 8, 2023

Conversation

SSHari
Copy link
Contributor

@SSHari SSHari commented Aug 3, 2023

PR to add an end-to-end test harness using Playwright.

Changes

  1. Added a Github Action to run the end to end tests
  2. Updated the sendMagicLinkEmail function to auto redirect to the magic link URL in test mode (Trying to parse the logs for the magic URL seemed unnecessary. We may consider just defaulting to this behavior in development in general.)
  3. Added a test nextjs app with a basic job to use with the e2e tests (the values in examples/nextjs-test/.env.example are associated with the test data)
  4. Added tests for creating an account and verifying jobs based on the test nextjs app from Change 3

Running Locally

To run the tests locally, the following section from the publish github action can be followed:

# Setup environment variables
cp ./.env.example ./.env
cp ./examples/nextjs-test/.env.example ./examples/nextjs-test/.env.local
cp ./packages/database/.env.example ./packages/database/.env
pnpm run docker
pnpm run db:migrate
pnpm run db:seed
pnpm run test:e2e
pnpm run docker:stop

Other

Since I can't verify the github action works until we merge this change into the main branch, I created a dummy github action with the same steps below that's triggered when my branch is updated. The results of that github action can be found here: https://github.com/SSHari/trigger.dev/actions/runs/5745732725/job/15574162405. One thing to note is that I couldn't run my test github action on the buildjet-4vcpu-ubuntu-2204 runner, so I used the ubuntu-22.04 instead.

Bounty: /claim #202

@changeset-bot
Copy link

changeset-bot bot commented Aug 3, 2023

⚠️ No Changeset found

Latest commit: b7ab082

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Copy link

@zodman zodman left a comment

Choose a reason for hiding this comment

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

amazing!!!

I think is missing the document of how to run the e2e in CONTRIBUITION.md file

give 💸 to this PR!

import { test, expect } from "@playwright/test";

test("Create an account", async ({ page }) => {
await page.goto("http://localhost:3030/login");
Copy link

Choose a reason for hiding this comment

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

move the URL to the playwright.config.ts use.baseURL. https://playwright.dev/docs/test-configuration

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added the base URL to the config file and updated the tests to use the relative paths.

});

test("Verify jobs from the test nextjs project", async ({ page }) => {
await page.goto("http://localhost:3030");
Copy link

Choose a reason for hiding this comment

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

Same here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added the base URL to the config file and updated the tests to use the relative paths.

Copy link

@zodman zodman Aug 3, 2023

Choose a reason for hiding this comment

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

I suggest use the Page Object Pattern for getting test more easy to maintain...

https://www.selenium.dev/documentation/test_practices/encouraged/page_object_models/

reporter: "html",
/* Shared settings for all the projects below. See https://playwright.dev/docs/api/class-testoptions. */
use: {
/* Collect trace when retrying the failed test. See https://playwright.dev/docs/trace-viewer */
Copy link

Choose a reason for hiding this comment

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

here is missing the baseURL and readed from .env file

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added the base URL to the config file and updated the tests to use the relative paths.

tests/utils.ts Outdated
},
});

try {
Copy link

Choose a reason for hiding this comment

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

remove the try .. if the connection not success the test should be fail ... ASAP..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed the try/catch

apps/webapp/package.json Show resolved Hide resolved
apps/webapp/app/services/email.server.ts Outdated Show resolved Hide resolved
examples/nextjs-test/package.json Outdated Show resolved Hide resolved
@ericallam
Copy link
Member

This is amazing, great work! Just a few tweaks and it should be ready to merge 👍

@SSHari
Copy link
Contributor Author

SSHari commented Aug 3, 2023

This is amazing, great work! Just a few tweaks and it should be ready to merge 👍

Hey @ericallam, I've gone ahead and made the suggested changes. I've also updated the CONTRIBUTING.md to outline how to run the end-to-end tests locally.

Also, I re-verifed the github actions based on the suggested changes in my forked repo which can be found here: https://github.com/SSHari/trigger.dev/actions/runs/5753854155/job/15597913604

Copy link
Member

@matt-aitken matt-aitken left a comment

Choose a reason for hiding this comment

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

This is awesome @SSHari, merging now. Thanks!

@matt-aitken matt-aitken dismissed ericallam’s stale review August 8, 2023 12:55

All issues resolved

@matt-aitken matt-aitken merged commit fa942fc into triggerdotdev:main Aug 8, 2023
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.

4 participants