Skip to content

test: Test 250+ popular npm packages#93

Merged
timfish merged 12 commits intonodejs:mainfrom
timfish:test/popular-modules
Jul 1, 2024
Merged

test: Test 250+ popular npm packages#93
timfish merged 12 commits intonodejs:mainfrom
timfish:test/popular-modules

Conversation

@timfish
Copy link
Copy Markdown
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
Copy Markdown
Member

@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.

@timfish
Copy link
Copy Markdown
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
@timfish timfish changed the title test: Test 240+ popular npm packages test: Test 250+ popular npm packages Jun 17, 2024
@timfish
Copy link
Copy Markdown
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
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
@timfish timfish deleted the test/popular-modules branch July 1, 2024 11:56
@github-actions github-actions bot mentioned this pull request Jul 6, 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.

5 participants