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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: CI workflow error & add separate reference files for different platforms #2633

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

wojtekmaj
Copy link
Contributor

@wojtekmaj wojtekmaj commented Feb 11, 2024

Follow up after #2631 馃珷

Fixes invalid syntax in workflow file.

Also, it turns out, generated PDFs differ slightly from platform to platform. As the previous reference file I generated comes from macOS, it fails when E2E is ran on linux. This fixes that by introducing platform-specific reference files.

Copy link

changeset-bot bot commented Feb 11, 2024

鈿狅笍 No Changeset found

Latest commit: e1f08df

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

@diegomura
Copy link
Owner

@wojtekmaj looks good but tests are failing

@wojtekmaj
Copy link
Contributor Author

wojtekmaj commented Feb 11, 2024

I can actually reproduce that locally. jay-peg seemed to have originally been released with the wrong configuration in package.json. You've fixed that in diegomura/jay-peg@d88f86c but in our lockfile, there's still an old unpatched version 1.0.0, causing the issue.

Patched it up locally and forgot about it 馃檲

You can bump the dependency and the tests will be green.

@wojtekmaj
Copy link
Contributor Author

Dependency bumped in #2635.

wojtekmaj added a commit to wojtekmaj/diegomura-react-pdf that referenced this pull request Feb 12, 2024
Turns out, generated PDFs differ slightly from platform to platform. As the previous reference file I generated comes from macOS, it fails when E2E is ran on linux. This fixes that by introducing platform-specific reference files.

Note: without diegomura#2633, E2E will error, and without diegomura#2635, CJS E2E will still fail.
wojtekmaj added a commit to wojtekmaj/diegomura-react-pdf that referenced this pull request Feb 12, 2024
Turns out, generated PDFs differ slightly from platform to platform. As the previous reference file I generated comes from macOS, it fails when E2E is ran on linux. This fixes that by introducing platform-specific reference files.

Note: without diegomura#2633, E2E will error, and without diegomura#2635, CJS E2E will still fail.
Turns out, generated PDFs differ slightly from platform to platform. As the previous reference file I generated comes from macOS, it fails when E2E is ran on linux. This fixes that by introducing platform-specific reference files.
@wojtekmaj wojtekmaj changed the title fix: CI workflow error - attempt 2 fix: CI workflow error & add separate reference files for different platforms Feb 12, 2024
@wojtekmaj
Copy link
Contributor Author

That's more like it

@wojtekmaj
Copy link
Contributor Author

@diegomura Sorry for pinging you, but can we prioritize this PR? Having all the other PRs without CI running is affecting us quite badly 馃檲

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.

None yet

2 participants