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: fix TAV tests for @cucumber/cucumber and @aws-sdk/client-s3 #1838

Merged
merged 7 commits into from
Dec 11, 2023

Conversation

trentm
Copy link
Contributor

@trentm trentm commented Nov 30, 2023

  • Switch to 'npm run --ws test-all-versions ...' for running TAV tests,
    instead of 'lerna run test-all-versions ...' because nx sets
    'npm_config_legacy_peer_deps=true' which breaks
    '@cucumber/cucumber@9.1.2' install and could break other installs by
    ignoring 'peerDependencies'.
  • Skip the bad '@aws-sdk/client-s3@3.377.0' release in TAV tests.

Also:

  • Reduce the number of versions of 'aws-sdk' and '@aws-sdk/*' packages
    test in TAV tests from 249, 143, and 132 versions currently, to 7
    each.
  • Add a top-level npm run test-all-versions script to run that script
    in all packages that have one. This is the equivalent of the
    "test-all-versions.yml" CI workflow.

Fixes: #1828


Update for reviewers: See #1838 (comment) below for why I think this PR should go in, even though TAV tests are still failing.

- Switch to 'npm run --ws test-all-versions ...' for running TAV tests,
  instead of 'lerna run test-all-versions ...' because nx sets
  'npm_config_legacy_peer_deps=true' which breaks
  '@cucumber/cucumber@9.1.2' install and could break other installs by
  ignoring 'peerDependencies'.
- Skip the bad '@aws-sdk/client-s3@3.377.0' release in TAV tests.

Also:
- Reduce the number of versions of 'aws-sdk' and '@aws-sdk/*' packages
  test in TAV tests from 249, 143, and 132 versions currently, to 7
  each.
- Add a top-level `npm run test-all-versions` script to run that script
  in all packages that have one. This is the equivalent of the
  "test-all-versions.yml" CI workflow.

Fixes: open-telemetry#1828
Copy link

codecov bot commented Nov 30, 2023

Codecov Report

Merging #1838 (e664c0c) into main (3cb2802) will increase coverage by 0.04%.
The diff coverage is n/a.

❗ Current head e664c0c differs from pull request most recent head 06427fb. Consider uploading reports for the commit 06427fb to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1838      +/-   ##
==========================================
+ Coverage   91.45%   91.49%   +0.04%     
==========================================
  Files         144      144              
  Lines        7406     7388      -18     
  Branches     1483     1474       -9     
==========================================
- Hits         6773     6760      -13     
+ Misses        633      628       -5     

see 8 files with indirect coverage changes

@trentm
Copy link
Contributor Author

trentm commented Nov 30, 2023

Note to reviewers: The TAV tests failed, but for a different reason, so I think the PR is good to go.

  1. The TAV tests for cucumber passed. Good.
  2. The TAV tests for @aws-sdk/client-sqs@3.462.0 failed (Bad, but a separate issue, see below), before even getting to the @aws-sdk/client-s3 tests that are being fixed here.

the client-sqs failure

> @opentelemetry/instrumentation-aws-sdk@0.37.0 test-all-versions
> tav

-- required packages ["@aws-sdk/client-sqs@3.462.0"]
-- installing ["@aws-sdk/client-sqs@3.462.0"]
-- running test "npm run test" with @aws-sdk/client-sqs (env: {})
...
  3 failing

  1) instrumentation-aws-sdk-v3
       custom service behavior
         SQS
           sqs send add messaging attributes:
     SyntaxError: Unexpected token < in JSON at position 0
  Deserialization error: to see the raw response, inspect the hidden field {error}.$response on this object.
      at JSON.parse (<anonymous>)
      at /home/runner/work/opentelemetry-js-contrib/opentelemetry-js-contrib/node_modules/@aws-sdk/client-sqs/dist-cjs/protocols/Aws_json1_0.js:1980:21
      at processTicksAndRejections (internal/process/task_queues.js:95:5)
      at de_SendMessageCommand (/home/runner/work/opentelemetry-js-contrib/opentelemetry-js-contrib/node_modules/@aws-sdk/client-sqs/dist-cjs/protocols/Aws_json1_0.js:1013:18)
      at /home/runner/work/opentelemetry-js-contrib/opentelemetry-js-contrib/node_modules/@smithy/middleware-serde/dist-cjs/deserializerMiddleware.js:7:24
      at /home/runner/work/opentelemetry-js-contrib/opentelemetry-js-contrib/node_modules/@aws-sdk/client-sqs/node_modules/@aws-sdk/middleware-signing/dist-cjs/awsAuthMiddleware.js:30:20
      at /home/runner/work/opentelemetry-js-contrib/opentelemetry-js-contrib/node_modules/@smithy/middleware-retry/dist-cjs/retryMiddleware.js:27:46
      at /home/runner/work/opentelemetry-js-contrib/opentelemetry-js-contrib/node_modules/@aws-sdk/middleware-sdk-sqs/dist-cjs/send-message.js:7:18
      at /home/runner/work/opentelemetry-js-contrib/opentelemetry-js-contrib/node_modules/@aws-sdk/client-sqs/node_modules/@aws-sdk/middleware-logger/dist-cjs/loggerMiddleware.js:7:26
...

While we have #1477 already for client-sqs TAV test failures... this failure is actually a different failure reason than the one currently being discussed there. I can update that issue (or create a separate one).

It would help if this PR went in, because the .tav.yml change means we are testing fewer versions of client-sqs so we should hit the test failures quicker in TAV test runs.

trentm added a commit that referenced this pull request Dec 6, 2023
…ient-sqs versions using the AWS JSON 1.0 protocol (#1844)

Versions 3.446.0 and later switched to a new JSON protocol.
https://github.com/aws/aws-sdk-js-v3/blob/main/clients/client-sqs/CHANGELOG.md#34460-2023-11-08

Refs: #1838 (comment)
trentm added a commit to trentm/opentelemetry-js-contrib that referenced this pull request Dec 6, 2023
Copy link
Member

@pichlermarc pichlermarc left a comment

Choose a reason for hiding this comment

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

Looks great, thanks for taking care of this 🙂

@trentm trentm merged commit a8f3a3d into open-telemetry:main Dec 11, 2023
11 of 14 checks passed
@trentm trentm deleted the tm-tav-fix-no-legacy-peer-deps branch December 11, 2023 18:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[ci] test-all-versions failing for @cucumber/cucumber@9.1.2 and @aws-sdk/client-s3@3.377.0
3 participants