-
Notifications
You must be signed in to change notification settings - Fork 579
discussion: how to npm test
with modern versions of instrumented packages?
#2722
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
Comments
…strumented packages This flips the usage of 'npm test' and 'npm run test-all-versions' so that the former uses recent versions of the instrumented packages. This allows updating devDeps to the latest. 'npm test' will *skip* tests for older versions of node that aren't supported by the latest version of the instrumented packages. Testing all the old versions (of node and instrumented packages) is left to 'npm run test-all-versions' (TAV). Refs: open-telemetry#2722
A possible other option for the instrumentation-aws-sdk case would have been to just drop Node 14 and 16 support for the instrumentation. However, I felt that was probably slightly premature. In general we shouldn't have to do this. |
Thanks @trentm for the help with this.
Just wanted to clarify that IIRC, '3.587.0' is actually the first version that included |
…strumented packages (#2723) This flips the usage of 'npm test' and 'npm run test-all-versions' so that the former uses recent versions of the instrumented packages. This allows updating devDeps to the latest. 'npm test' will skip tests for older versions of node that aren't supported by the latest version of the instrumented packages. Testing all the old versions (of node and instrumented packages) is left to 'npm run test-all-versions' (TAV). Refs: #2722
#2723 is merged now, which implements the proposal for instr-aws-sdk. We can do similar for other instrumentations when helpful/convenient. |
…strumented packages (open-telemetry#2723) This flips the usage of 'npm test' and 'npm run test-all-versions' so that the former uses recent versions of the instrumented packages. This allows updating devDeps to the latest. 'npm test' will skip tests for older versions of node that aren't supported by the latest version of the instrumented packages. Testing all the old versions (of node and instrumented packages) is left to 'npm run test-all-versions' (TAV). Refs: open-telemetry#2722
This PR starts with @timfish's open-telemetry#2437 (Tim did all the hard work) and makes it a smaller patch -- mostly by avoiding having separate test/v4 and test/v5 trees. Now that we are on JS SDK v2 and hence only support Node.js v18 as a minimum (the same as express@5) the testing story is now simpler: no need to avoid running some tests with older Node.js versions. (See open-telemetry#2722 for a general discussion of the pain when having to do that.) Obsoletes: open-telemetry#2437 Closes: open-telemetry#2435
A current issue that hinders development of some of the instrumentations happens when the package being instrumented (e.g. aws-sdk v3, pino) drops support for a version of Node.js, but the instrumentation still supports the older version. When this happens, currently our instrumentations will freeze their devDependency to the older version of the instrumented package, so that
npm test
can work with the older node.concrete examples
instrumentation-pino
supports pino@9 (the supported range is>=5.14.0 <10
). pino@9 supports only Node.js 18 and later, but instrumentation-pino supports Node.js>=14
. CI runsnpm test
with Node.js 14, 16, etc. For those tests to pass the pino version in instrumentation-pino/package.json needs to be frozen to the older pino@8 major version. (feat(pino): support new pino version ^9.0.0 #2163 added pino@9 support, but left the devDep at pino@8.)instrumentation-aws-sdk
supports a large range of aws-sdk v3 versions. Modern releases of@aws-sdk/client-*
packages support only Node.js 18 and later, butinstrumentation-aws-sdk
supports Node.js 14 and later. Currently the aws-sdk v3 devDeps are frozen at versions released in 2022.the dev pain
In general, this makes developing the instrumentations for more modern versions of the instrumented packages a little more difficult. One has to know to
npm install --no-save ...
the more modern versions for local dev/testing.A more recent difficulty caused by this with
instrumentation-aws-sdk
is #2700 that is adding instrumentation for@aws-sdk/client-bedrock-runtime
. The author reasonably added a devDependency on the current release of this package... one that only supports Node.js 18 and later. The current frozen dep of the other@aws-sdk/client-*
packages ("3.85.0"), does not exist for this new AWS client package. Options are:@aws-sdk/client-bedrock-runtime
that supports Node.js 14 and use that. There happen to be some releases (e.g.'3.565.0': '2024-04-29T19:11:29.197Z',
and earlier). However, (a) it is pretty difficult to go back and find an appropriate version; (b) this is to support running on Node.js 14 and 16, which is getting harder and harder to care about; and (c) in general there could be a newer@aws-sdk/client-*
package that only started after Node 16 support was dropped.test/client-bedrock-runtime.test.ts
when running with Node 14 or 16. This is remarkably painful to do with mocha. More details later. Another problem is that having both old and new@aws-sdk/
devDeps, e.g.:results in a gargantuan package-lock.json diff and install size because multiple separate trees of the large aws-sdk transitive dep chain are installed.
proposal
I propose that, within reason, we move towards having
npm test
test against modern deps, and leave testing with older versions to test-all-versions (TAV). This means:npm test
in a given instrumentation package might be a no-op for old versions of Node.jsnpm run test-all-versions
would run the appropriate subset of tests that can run with an older Node.js.The trick is finding a reasonable way to do this with
mocha
. I found this difficult. I have a coming PR (for instrumentation-aws-sdk) that demonstrates my current idea.The text was updated successfully, but these errors were encountered: