Skip to content

Conversation

@yusukebe
Copy link
Owner

Currently, the e2e test tests the app running on only Vite. This PR enables testing with the Wrangler command. This means we can test handlers for Cloudflare Workers and Cloudflare Pages.

@yusukebe
Copy link
Owner Author

Hey @ogadra ! Can you review this?

"typecheck": "tsc",
"preview": "npm run build && wrangler dev"
"preview": "npm run build && wrangler dev",
"test:e2e:workers": "npm run build && playwright test -c playwright-workers.config.ts e2e.test.ts"
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be kinder to add examples/cloudflare-workers/playwright-vite.config.ts and this line ↓

"test:e2e:vite:workers": "playwright test -c playwright-vite.config.ts e2e.test.ts",

Copy link
Owner Author

Choose a reason for hiding this comment

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

If I added the test for vite, it will test the same things as the test in examples/cloudflare-pages. So, I didn't add the test for examples/cloudflare-workers. But, as you feel, it looks weird why it does not test for Vite in examples/cloudflare-workers, and it is possible that the example will be changed. So, I'll add a vite test.

However, the naming of test is test:e2e:vite is okay since this name is used only in examples/cloudflare-workers project.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Added the test for Vite: d92c8c2

"typecheck": "tsc",
"preview": "npm run build && wrangler pages dev",
"test:e2e": "playwright test -- -c playwright.config.ts e2e.test.ts"
"test:e2e:vite": "playwright test -c playwright-vite.config.ts e2e.test.ts",
Copy link
Contributor

Choose a reason for hiding this comment

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

If you add the test:e2e:vite:workers command, you might need to change this line.

Suggested change
"test:e2e:vite": "playwright test -c playwright-vite.config.ts e2e.test.ts",
"test:e2e:vite:pages": "playwright test -c playwright-vite.config.ts e2e.test.ts",

Copy link
Owner Author

Choose a reason for hiding this comment

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

With the same reason above, the change is unnecessary because this name is used only the project.

"test:e2e": "npm run test:e2e:cloudflare-pages",
"test:e2e:cloudflare-pages": "npm run test:e2e -w example-cloudflare-pages",
"test:e2e": "npm-run-all -s test:e2e:*",
"test:e2e:vite": "npm run test:e2e:vite -w example-cloudflare-pages",
Copy link
Owner Author

Choose a reason for hiding this comment

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

Anyway, I think it's okay to add the Vite test only in example-cloudflare-pages, not including example-cloudflare-workers's since the test can cover all tests for the Vite.

@yusukebe
Copy link
Owner Author

@ogadra

Thank you for the review. This seems to be good. Merging.

@yusukebe yusukebe merged commit 1e50149 into main Nov 20, 2024
2 checks passed
@yusukebe yusukebe deleted the test/test-with-wrangler branch November 20, 2024 19:22
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.

3 participants