Skip to content

ci: add PDF export check - #6

Closed
hiyouga wants to merge 1 commit into
mainfrom
codex/add-pdf-export-ci
Closed

ci: add PDF export check#6
hiyouga wants to merge 1 commit into
mainfrom
codex/add-pdf-export-ci

Conversation

@hiyouga

@hiyouga hiyouga commented May 20, 2026

Copy link
Copy Markdown
Contributor

Summary

  • add a Playwright smoke test that exports the default deck to PDF
  • upload the generated PDF as a CI artifact
  • add Playwright config and ignore generated test artifacts

Verification

  • npm run build
  • PDF_EXPORT_DIR=pdf-export npm run test:pdf
  • BASE_PATH=/minimal-web-slides/ npm run build

Copilot AI review requested due to automatic review settings May 20, 2026 07:13

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request introduces end-to-end testing for PDF exports using Playwright, adding the necessary dependencies, configuration, and a test suite. The reviewer suggested increasing the global test timeout to provide a buffer for browser overhead and recommended using regex patterns for filename assertions to make the tests more resilient to locale-specific variations.

Comment thread playwright.config.js

export default defineConfig({
testDir: "./tests",
timeout: 120_000,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

The global test timeout (120s) is identical to the specific timeout for the download event in the test script. This can cause the test to fail prematurely due to the overhead of browser startup and navigation. Increasing the global timeout provides a necessary buffer for teardown and reporting.

Suggested change
timeout: 120_000,
timeout: 150_000,

Comment thread tests/pdf-export.spec.js

const PDF_HEADER = "%PDF-";
const PDF_EXPORT_DIR = process.env.PDF_EXPORT_DIR || "pdf-export";
const EXPECTED_FILENAME = "minimal-web-slides-zh.pdf";

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

Hardcoding the expected filename to include -zh makes the test dependent on the environment's default locale. It's more robust to use a pattern check for the filename to ensure the test passes regardless of the browser's language settings.

Suggested change
const EXPECTED_FILENAME = "minimal-web-slides-zh.pdf";
const EXPECTED_FILENAME_PATTERN = /^minimal-web-slides-.*\.pdf$/;

Comment thread tests/pdf-export.spec.js
Comment on lines +20 to +24
expect(download.suggestedFilename()).toBe(EXPECTED_FILENAME);

const outputDir = path.resolve(PDF_EXPORT_DIR);
const outputPath = path.join(outputDir, EXPECTED_FILENAME);
await mkdir(outputDir, { recursive: true });

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

Instead of relying on a hardcoded filename for both assertion and saving, use the filename suggested by the browser. This ensures the test correctly verifies the application's output and saves the file with the actual name generated, making the test more resilient to changes in naming logic or locale.

Suggested change
expect(download.suggestedFilename()).toBe(EXPECTED_FILENAME);
const outputDir = path.resolve(PDF_EXPORT_DIR);
const outputPath = path.join(outputDir, EXPECTED_FILENAME);
await mkdir(outputDir, { recursive: true });
const suggestedFilename = download.suggestedFilename();
expect(suggestedFilename).toMatch(EXPECTED_FILENAME_PATTERN);
const outputDir = path.resolve(PDF_EXPORT_DIR);
const outputPath = path.join(outputDir, suggestedFilename);
await mkdir(outputDir, { recursive: true });

@hiyouga hiyouga closed this May 20, 2026

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Adds a Playwright-based CI smoke test to validate that the default slide deck can be exported to a PDF and publishes the generated PDF as a workflow artifact, helping catch regressions in the PDF export flow.

Changes:

  • Added a Playwright test that triggers “Export PDF”, saves the download, and performs basic PDF validation.
  • Added Playwright configuration (including a webServer) and wired a new test:pdf npm script.
  • Updated CI to install the Chromium browser for Playwright, run the export test, and upload the exported PDF artifact; ignored generated Playwright/PDF artifacts in .gitignore.

Reviewed changes

Copilot reviewed 4 out of 6 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
tests/pdf-export.spec.js New Playwright smoke test that exports and validates a PDF download.
playwright.config.js New Playwright config with baseURL, timeouts, and a Vite preview webServer.
package.json Adds Playwright dev dependency and a test:pdf script.
package-lock.json Locks Playwright-related dependencies.
.gitignore Ignores Playwright reports/results and the pdf-export/ output directory.
.github/workflows/ci.yml Installs Playwright Chromium, runs the PDF export test, and uploads the PDF artifact.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread package.json
"build": "vite build",
"preview": "vite preview --host 0.0.0.0"
"preview": "vite preview --host 0.0.0.0",
"test:pdf": "playwright test tests/pdf-export.spec.js"
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