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

chore: add basic E2E tests #2628

Merged
merged 1 commit into from
Feb 9, 2024
Merged

chore: add basic E2E tests #2628

merged 1 commit into from
Feb 9, 2024

Conversation

wojtekmaj
Copy link
Contributor

@wojtekmaj wojtekmaj commented Feb 9, 2024

This proposal adds E2E tests. It runs these tests on Node.js 18, 20, and 21 (so just like unit tests), in CJS and ESM mode.

Each test creates a stupid simple PDF and compares it to a reference PDF file stored inside the repository. It's purposefully minimal: the aim of these E2E tests is to catch any bugs that would break module resolution on different Node.js environments, rather than test advanced react-pdf rendering features, which is beautifully done in unit tests already.

Would have prevented #2602, #2608. Here's what happens if I remove interop: 'compat' bit added in #2605 from @react-pdf/renderer's Rollup config:

% yarn --cwd e2e/node-esm start
yarn run v1.22.19
$ node ./index.js
✔ rendering a PDF (22.69725ms)
ℹ tests 1
ℹ suites 0
ℹ pass 1
ℹ fail 0
ℹ cancelled 0
ℹ skipped 0
ℹ todo 0
ℹ duration_ms 27.5615
✨  Done in 0.28s.

⬆️ ESM is fine. But it was never broken in the first place.

% yarn --cwd e2e/node-cjs start
yarn run v1.22.19
$ node ./index.js
/react-pdf/packages/renderer/lib/react-pdf.cjs:4178
var fontStore = new FontStore();
                ^

TypeError: FontStore is not a constructor
    at Object.<anonymous> (/react-pdf/packages/renderer/lib/react-pdf.cjs:4178:17)
    at Module._compile (node:internal/modules/cjs/loader:1241:14)
    at Module._extensions..js (node:internal/modules/cjs/loader:1295:10)
    at Module.load (node:internal/modules/cjs/loader:1091:32)
    at Module._load (node:internal/modules/cjs/loader:938:12)
    at Module.require (node:internal/modules/cjs/loader:1115:19)
    at require (node:internal/modules/helpers:130:18)
    at Object.<anonymous> (/react-pdf/e2e/node-cjs/index.js:5:50)
    at Module._compile (node:internal/modules/cjs/loader:1241:14)
    at Module._extensions..js (node:internal/modules/cjs/loader:1295:10)

Node.js v20.9.0
error Command failed with exit code 1.

⬆️ Whooops there we go! Caught CJS setup error.

Will prevent any future issues with module resolution caused by e.g. dependency updates or Rollup config updates.

Closes #2611

PS. I'm considering another E2E test that would spin up Vite-based front-end application, open it in the browser using Playwright and do a similar comparison, although that's a bit too much work for me at the moment.

This proposal adds E2E tests. It runs these tests on Node.js 18, 20, and 21 (so just like unit tests), in CJS and ESM mode.

Each test creates a stupid simple and compares it to reference PDF file stored inside the repository. It's purposefully minimal: the aim of these E2E tests is to catch any bugs that would break module resolution on different Node.js environments, rather than test advanced react-pdf rendering features, which is beautifully done in unit tests already.

Would have prevented #2602, #2608.

Will prevent any future issues with module resolution caused by e.g. dependency updates or Rollup config updates.

Closes #2611

PS. I'm considering another E2E test that would spin up Vite-based front-end application, opens it using Playwright and does a similar comparison, although that's a bit too much work for me at the moment.
Copy link

changeset-bot bot commented Feb 9, 2024

⚠️ No Changeset found

Latest commit: e7235d8

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
Owner

@diegomura diegomura left a comment

Choose a reason for hiding this comment

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

utACK

Nice! Thanks (again 😄 ) @wojtekmaj for all the hard work. I'll try to catch up during the weekend with all the threads you commented during the week 😄

@diegomura diegomura merged commit 021c354 into diegomura:master Feb 9, 2024
@wojtekmaj wojtekmaj deleted the e2e branch February 9, 2024 23:33
mskec pushed a commit to mskec/react-pdf that referenced this pull request Feb 26, 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.

Implement tests testing bundled code
2 participants