Skip to content

test(e2e): Add Node SDK re-exports consistency e2e test #10389

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

Merged
merged 1 commit into from
Jan 29, 2024

Conversation

Lms24
Copy link
Member

@Lms24 Lms24 commented Jan 29, 2024

As notified in #10384, we were missing quite a few exports in the Remix SDK. I fixed a couple of them in #10385 but wanted to do a proper audit of other SDKs where we have the same potential problem.

As a result, this PR introduces an e2e test that checks if all @sentry/node exports were re-exported in packages that depend on @sentry/node. It's not a real e2e test, as there's no actual test application, but we only execute a script that imports various SDKs depending on @sentry/node. However, IMO it makes a lot of sense to test the export of the final tarballs which we already have the infrastructure for in our e2e tests.

Some remarks:

  • Of course there are special cases around what actually should be re-exported. The test script allows to handle this on a global, as well as dependent-specific basis.
  • Limitation: We only test top-level exports for now (IMO this is enough).
  • Limitation: We only test ESM (I think; again probably good enough)
  • Limitation: Bun doesn't work yet, still looking into it (might fix in a follow-up PR)

Example of failing CI job:

image

Background:

For reasons unknown to us, rollup seems to bundle export * from "@sentry/node" statements into the default property of higher level SDKs. Therefore we have to individually re-export APIs from @sentry/node in each SDK depending on the Node package. This has a lot of potential for unintended export inconsistencies.

I'm still skipping some packages on purpose as these currently actually still have missing exports. Will fix them in follow-up PRs and opt them in to this test. Decided on this order because this makes my decisions clearer as to what we actually re-export in the packages.

Copy link
Contributor

@lforst lforst left a comment

Choose a reason for hiding this comment

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

very cool

@Lms24 Lms24 merged commit 78c6a9d into develop Jan 29, 2024
@Lms24 Lms24 deleted the lms/test-e2e-node-exports-test branch January 29, 2024 14:57
Lms24 added a commit that referenced this pull request Jan 30, 2024
Missed some exports in my manual pass in #10385. Test in #10389
discovered more missing exports which this PR adds or marks as
unnecessary in the re-export test.
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