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

Attempting to fix Playwright tests failing - human edition #1884

Draft
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

stolinski
Copy link
Collaborator

@stolinski stolinski commented Nov 1, 2024

Copy link

vercel bot commented Nov 1, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
syntax-website ❌ Failed (Inspect) Nov 1, 2024 9:10pm

Copy link

codecov bot commented Nov 1, 2024

Bundle Report

Changes will increase total bundle size by 5.46kB (0.02%) ⬆️. This is within the configured threshold ✅

Detailed changes
Bundle name Size Change
website-client-esm 2.15MB 4.11kB (0.19%) ⬆️
website-server-esm 30.59MB 1.36kB (0.0%) ⬆️

Copy link

@mxschmitt mxschmitt left a comment

Choose a reason for hiding this comment

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

Hey Scott! Love your Podcast! Was listening to nearly every episode when I started with development in 2018/19 - I'm sure it shaped me a lot, thank you!

I made a couple of comments but they are all unrelated to the actual issue you were reporting in microsoft/playwright#33403 which triggered the Segmentation fault (core dumped). I think this is not related to Playwright but took this opportunity to give you a few best-practises on the way.

@@ -42,7 +42,7 @@ jobs:
run: pnpm db:generate

- name: Install playwright browsers
run: pnpx playwright install --with-deps
run: pnpm exec playwright install --with-deps

Choose a reason for hiding this comment

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

Suggested change
run: pnpm exec playwright install --with-deps
run: pnpm exec playwright install --with-deps chromium

That should make it much faster (60s -> 15s -ish)

@@ -28,12 +28,12 @@
"stylelint": "NODE_OPTIONS=--no-deprecation stylelint \"**/*.css\""
},
"devDependencies": {
"@codecov/sveltekit-plugin": "0.0.1-beta.12",
"@codecov/sveltekit-plugin": "1.2.1",

Choose a reason for hiding this comment

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

line 27 (perf): should be: pnpm exec playwright install chromium.

import type { PlaywrightTestConfig } from '@playwright/test';

const config: PlaywrightTestConfig = {
const config = {
Copy link

@mxschmitt mxschmitt Nov 1, 2024

Choose a reason for hiding this comment

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

why this change? we usually recommend a couple of defaults be overridden which give you better performance over time. Thinking about:

import { defineConfig, devices } from '@playwright/test';

const config = defineConfig({
  fullyParallel: true,
  retries: process.env.CI ? 2 : 0,
  webServer: {
    command: 'pnpm build:svelte && pnpm preview',
    port: 4173,
    timeout: 600_000
  },
  projects: [
    {
      name: 'chromium',
      use: { ...devices['Desktop Chrome'] },
    },
  ],
  testDir: 'tests'
});

export default config;
  • fullyParallel executes tests in parallel
  • retries: 2 on CI is usually a good practise since CI environments are usually considered flaky e.g. network issues or overloaded machine
  • projects: having project there makes it more clear.

For the future adding HTML reporter might also make sense and upload the report to GitHub if you want to be more advanced, see here for our guide.

@stolinski stolinski changed the title playwright fix human Attempting to fix Playwright tests failing - human edition Nov 4, 2024
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.

2 participants