test(instrumentation-aws-sdk): update devDeps to modern version of instrumented packages

[trentm]

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

skipping tests on older Node.js versions is awkward

The mechanism proposed here to skip running tests in npm test for older Node.js versions is to use a preload script via mocha --require ... (as is already being used for TS-to-JS transpilation) and a config envvar to state the min supported Node.js version. Effectively this:

SKIP_TEST_IF_NODE_OLDER_THAN=18 mocha --require '../../../scripts/skip-test-if.js' 'test/**/*.test.ts'

That preload "skip-test-if.js" will process.exit(0) to skip tests.
I'll mention some alternatives attempted in comments below.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.86%. Comparing base (4b51e60) to head (0c772ec).
Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2723      +/-   ##
==========================================
+ Coverage   91.53%   91.86%   +0.33%     
==========================================
  Files         171      171              
  Lines        8172     8172              
  Branches     1660     1660              
==========================================
+ Hits         7480     7507      +27     
+ Misses        692      665      -27     

see 2 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[trentm]

alternative attempted: top-level "skip this test file" logic

An alternative I've used before is to have a top-level if-block in a give test file that just exited if the node version was too old for the given version of the module, e.g.:
https://github.com/elastic/apm-agent-nodejs/blob/fc37783b9e5ae51b64cc5d7cd89a2aa35c4aa89f/test/instrumentation/modules/mysql2/mysql.test.js#L19-L25

However, this cannot work with mocha because it runs all test in process.

alternative attempted: load dep in try/catch

I made another attempt to load the devDep in a try/catch, then have each mocha test block this.skip() in beforeEach if the expected module vars weren't loaded.

Example diff

--- a/plugins/node/opentelemetry-instrumentation-aws-sdk/test/s3.test.ts
+++ b/plugins/node/opentelemetry-instrumentation-aws-sdk/test/s3.test.ts
@@ -22,7 +22,17 @@ import { AwsInstrumentation } from '../src';
 import { AttributeNames } from '../src/enums';
 registerInstrumentationTesting(new AwsInstrumentation());

-import { PutObjectCommand, S3Client } from '@aws-sdk/client-s3';
+// Skip tests, rather than crashing, if this import fails.
+let PutObjectCommand: unknown;
+let S3Client: unknown;
+try {
+  const mod = require('@aws-sdk/client-s3');
+  PutObjectCommand = mod.PutObjectCommand;
+  S3Client = mod.S3Client;
+} catch {
+  // Assuming the failure is due to this Node.js and aws-sdk being incompatible.
+}
+
 import * as fs from 'fs';
 import * as nock from 'nock';

@@ -33,6 +43,14 @@ import { expect } from 'expect';
 const region = 'us-east-1';

 describe('S3 - v3', () => {
+  beforeEach(function () {
+    if (!S3Client) {
+      // Skip because, presumably, the current version of node is too low to
+      // to support this version of `@aws-sdk/client-s3`.
+      this.skip();
+    }
+  });
+
   describe('PutObject', () => {
     it('Request span attributes - adds bucket Name', async () => {
       const dummyBucketName = 'ot-demo-test';

This is code-heavy because of the scoping of the loaded vars. Also potentially having to add the beforeEach() logic to multiple top-level describe()-blocks would be labourious.

The main hurdle that stopped me here was when types are imported from the instrumented module, e.g.:

opentelemetry-js-contrib/plugins/node/opentelemetry-instrumentation-aws-sdk/test/aws-sdk-v3.test.ts

Line 30 in 8a5c214

PutObjectCommandOutput,

Note that in the diff above I am using require() to load in a try/catch. Assuming I'm not missing something, to import a type and use it in a TS file, I'd need to use the import-statement, and this cannot be in a try/catch. So I'm stuck.

[trentm]

^ hup the pkg: label to get TAV tests to run.

[blumamir]

Suggesting an alternative.

In the past, we used to have something like this in the github workflow yaml:

      matrix:
        node: ["14", "16", "18", "20", "22"]
        include:
          - container: "14"
            lerna-extra-args: >-
              --ignore @opentelemetry/instrumentation-aws-sdk

and then when executing the test, we run them with this flag:

npm run test:ci:changed -- ${{ matrix.lerna-extra-args }}

This would prevent running the tests for specific packages on incompatible node versions (only in CI but that can be good enough until we drop support for old node versions).

[trentm]

This would prevent running the tests for specific packages on incompatible node versions (only in CI but that can be good enough until we drop support for old node versions)

We discussed this a while back offline. My argument against this was:

• I'd prefer, where possible, to not have CI-only things to have tests work. As long as package.json#engines.node says Node.js v16 is supported, I think a developer should (within reason) be able to use Node.js v16 and have npm test pass (it may pass because it just skips all tests, but that's better than blowing up).
• True that we are going to drop node v14 and 16 support soon -- so these changes for instr-aws-sdk won't be needed for very long. However, IIUC, some libs like pino and fastify have already dropped support for Node.js v18 in their latest versions. Therefore, this skip-test-if technique could be useful for instrumentation-pino and instrumentation-fastify.

[trentm]

Now that I fixed usage of SKIP_TEST_IF_DISABLE=true in .tav.yml, there is a new error with npm run test-all-versions and Node.js 14. See https://github.com/open-telemetry/opentelemetry-js-contrib/actions/runs/13844159477/job/38738994167?pr=2723

Reproduced locally:

% cd plugins/node/opentelemetry-instrumentation-aws-sdk
% nvm use 14
% node --version
v14.21.3

% npm install npm@9    # as is done in CI to get an npm that can handle monorepos
% ./node_modules/.bin/npm --version
9.9.4

% TAV=@aws-sdk/client-sqs ./node_modules/.bin/npm run test-all-versions

> @opentelemetry/[email protected] test-all-versions
> tav

-- required packages ["@aws-sdk/[email protected]"]
-- installing ["@aws-sdk/[email protected]"]
-- running test "npm run test" with @aws-sdk/client-sqs (env: { SKIP_TEST_IF_DISABLE: 'true' })

> @opentelemetry/[email protected] test
> SKIP_TEST_IF_NODE_OLDER_THAN=18 nyc mocha --require '../../../scripts/skip-test-if.js' --require '@opentelemetry/contrib-test-utils' 'test/**/*.test.ts'


 Exception during run: /Users/trentm/tm/opentelemetry-js-contrib7/node_modules/@aws-sdk/client-s3/dist-cjs/index.js:322
  static {
         ^

SyntaxError: Unexpected token '{'
    at wrapSafe (internal/modules/cjs/loader.js:1029:16)
    at Module._compile (internal/modules/cjs/loader.js:1078:27)
    at Module.replacementCompile (/Users/trentm/tm/opentelemetry-js-contrib7/node_modules/append-transform/index.js:60:13)
    at Module._extensions..js (internal/modules/cjs/loader.js:1143:10)
    at Object.<anonymous> (/Users/trentm/tm/opentelemetry-js-contrib7/node_modules/append-transform/index.js:64:4)
    at Module.load (internal/modules/cjs/loader.js:979:32)
    at Function.Module._load (internal/modules/cjs/loader.js:819:12)
    at Module.require (internal/modules/cjs/loader.js:1003:19)
    at Module.patchedRequire (/Users/trentm/tm/opentelemetry-js-contrib7/node_modules/require-in-the-middle/index.js:256:27)
    at Module.Hook._require.Module.require (/Users/trentm/tm/opentelemetry-js-contrib7/node_modules/require-in-the-middle/index.js:181:27)
    at require (internal/modules/cjs/helpers.js:107:18)
    at Object.<anonymous> (/Users/trentm/tm/opentelemetry-js-contrib7/plugins/node/opentelemetry-instrumentation-aws-sdk/test/aws-sdk-v3.test.ts:28:1)
...

The issue here is that each of the AWS SDK JS v3 clients (client-sqs, client-s3, ...) are being testing separately, but all using npm test, which will run tests for the other clients. What happens is:

• npm ci installs the latest version of @aws-sdk/client-sqs and @aws-sdk/client-s3 -- versions that only support Node.js 18 and later.
• TAV (test-all-versions) testing of client-sqs installs a version of client-sqs that works with Node.js v14, then calls npm test.
• npm test runs test/aws-sdk-v3.test.ts, which imports @aws-sdk/client-sqs (works) and @aws-sdk/client-s3 (fails because this is still a recent version of the package that doesn't support Node.js 14)

Options:

1. Break up all the test files to only test a single client at a time and set the commands: in ".tav.yml" to test the particular files relevant for the client package under test.
2. Attempt to use peerDependencies option in the relevant .tav.yml group, to install an old enough version of the other client packages that will be run, but aren't really under test.
3. Give up on the skip-test-if technique just for instr-aws-sdk because (a) this new issue is specific to instr-aws-sdk (we won't hit this in instr-pino or instr-fastify) and (b) we'll be dropping node 14 and 16 support next week (with chore!: update to JS SDK 2.x #2738).

Option 2 is certainly easier than option 1, if it works, but less "correct".

[trentm]

Options:

1. Break up all the test files to only test a single client at a time and set the `commands:` in ".tav.yml" to test the particular files relevant for the client package under test.

commit 122a8fe does this option 1.

[trentm]

Reviewer note: Now that .tav.yml is running this test file alone (mocha ... test/sqs.test.ts), instead of with all the other test files (in the same process), this now needs to set AWS_ auth-related envvars to avoid a client error. When running all the test files (mocha ... 'test/**/*.test.ts'), then those envvars were being set in "test/aws-sdk-v3.test.ts" and solving the auth-related issue for all the test files).

[trentm]

Reviewer note: "test/aws-sdk-v3.test.ts" was broken into separate test files that each only test a single @aws-sdk/client-* client. This allows TAV (test-all-versions) to work properly when it is varying the installed version of a single client package at a time.