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

test: Test 250+ popular npm packages #93

Merged
merged 12 commits into from
Jul 1, 2024

Conversation

timfish
Copy link
Contributor

@timfish timfish commented Jun 2, 2024

Adds a script that tests 250+ of the most popular npm packages. It compares the exports without and with the loader hook and fails if they don't match. It also fails if errors are thrown with the loader hook in use.

Copy link

@AbhiPrasad AbhiPrasad left a comment

Choose a reason for hiding this comment

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

Nice!

We're not adding this to CI yet? We can just comment out the packages that don't work for now.

Aside from these popular npm packages, could we also test against some of the popular web frameworks? express, fastify, hapi, connect, sveltekit, nextjs, gatsby, remix. These are the most common places where users will use APM instrumentation, so I think it's important we catch any issues asap.

test/check-exports/test.mjs Outdated Show resolved Hide resolved
test/check-exports/test.mjs Outdated Show resolved Hide resolved
test/check-exports/test.mjs Outdated Show resolved Hide resolved
test/check-exports/test.mjs Outdated Show resolved Hide resolved
@timfish
Copy link
Contributor Author

timfish commented Jun 4, 2024

We're not adding this to CI yet? We can just comment out the packages that don't work for now.

This can be added to CI, but likely not for every Node version that gets tested because some of these libs won't support such a wide range. These tests with then potentially break often as the versions aren't pinned and they will update their supported node versions.

Maybe it's better the check in the package.json to make them more reliable?

could we also test against some of the popular web frameworks? express, fastify, hapi, connect, sveltekit, nextjs, gatsby, remix. These are the most common places where users will use APM instrumentation, so I think it's important we catch any issues asap.

Yeah it's certainly worth adding more to this list. Initially this was to find potential gaps in the existing unit tests.

If we pin the versions in a package.json, we will not get new failures when libraries break.

bengl pushed a commit that referenced this pull request Jun 14, 2024
Closes #95

I've also made the `getExports` functions return `Set<string>` instead
of `string[]` as this saves a load of unnecessary allocations.

I've also run this through the additional module tests in #93 and get
the same results.
bengl
bengl previously requested changes Jun 14, 2024
test/check-exports/test.mjs Outdated Show resolved Hide resolved
@timfish timfish changed the title test: Test 240+ popular npm packages test: Test 250+ popular npm packages Jun 17, 2024
@timfish
Copy link
Contributor Author

timfish commented Jun 17, 2024

I've now added this to CI testing and you can see it running in my fork.

@timfish timfish requested review from AbhiPrasad and bengl June 17, 2024 14:44
AbhiPrasad
AbhiPrasad previously approved these changes Jun 17, 2024
test/check-exports/test.mjs Outdated Show resolved Hide resolved
Qard
Qard previously approved these changes Jun 17, 2024
Co-authored-by: James Sumners <jsumners@newrelic.com>
jsumners-nr
jsumners-nr previously approved these changes Jun 18, 2024
@timfish timfish requested a review from jsumners-nr June 29, 2024 11:34
@timfish timfish requested a review from Qard June 29, 2024 11:34
@timfish timfish dismissed bengl’s stale review July 1, 2024 11:56

changes have been made

@timfish timfish merged commit 3b25c63 into nodejs:main Jul 1, 2024
48 checks passed
@timfish timfish deleted the test/popular-modules branch July 1, 2024 11:56
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

5 participants