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(instr-undici): fix TAV failures with Node.js 14 and 16 #2111

Merged
merged 1 commit into from
Apr 18, 2024

Conversation

trentm
Copy link
Contributor

@trentm trentm commented Apr 18, 2024

Test all versions (TAV) tests were failing with Node.js 14 and 16
because of subtle behaviour changes in socket end handling in
Node.js and undici for a test that tries a bogus request. This
restores earlier work-around code mistakenly removed in #2085.


Since #2085, TAV tests for Node.js 14 and 16 started failing. E.g.: https://github.com/open-telemetry/opentelemetry-js-contrib/actions/runs/8724672054/job/23935801386

> @opentelemetry/instrumentation-undici@0.1.0 test-all-versions
> tav

-- required packages ["undici@5.28.4"]
-- installing ["undici@5.28.4"]
-- running test "npm run test" with undici (env: {})

> @opentelemetry/instrumentation-undici@0.1.0 test
> nyc ts-mocha -p tsconfig.json test/**/*.test.ts

...
   UndiciInstrumentation `undici` tests
    disable()
      ✓ should not create spans when disabled
    enable()
      ✓ should ignore requests based on the result of ignoreRequestHook
      1) should create valid spans for different request methods
      ✓ should create valid spans for "request" method
      - should create valid spans for "fetch" method
      ✓ should create valid spans for "stream" method
      ✓ should create valid spans for "dispatch" method
      ✓ should create valid spans even if the configuration hooks fail
      ✓ should not create spans without parent if required in configuration
      ✓ should create spans with parent if required in configuration
      ✓ should capture errors while doing request (41ms)
      - should capture error if undici request is aborted


  9 passing (165ms)
  12 pending
  1 failing

  1) UndiciInstrumentation `undici` tests
       enable()
         should create valid spans for different request methods:
     SocketError: other side closed
      at Socket.onSocketEnd (node_modules/undici/lib/client.js:1118:22)
      at endReadableNT (internal/streams/readable.js:1333:12)
      at processTicksAndRejections (internal/process/task_queues.js:82:21)

Back in #2085 (comment) a similar issue with undici + older Node.js versions + this testing of a bogus request was hit and I mistakenly assumed it was the same issue. The workaround for that other issue did not fix this one. This PR restores the same/similar workaround.

Test all versions (TAV) tests were failing with Node.js 14 and 16
because of subtle behaviour changes in socket end handling in
Node.js and undici for a test that tries a bogus request. This
restores earlier work-around code mistakenly removed in open-telemetry#2085.
@trentm trentm self-assigned this Apr 18, 2024
@trentm trentm requested a review from a team April 18, 2024 04:42
Copy link

codecov bot commented Apr 18, 2024

Codecov Report

Merging #2111 (9ab8a55) into main (dfb2dff) will decrease coverage by 0.21%.
Report is 56 commits behind head on main.
The diff coverage is n/a.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2111      +/-   ##
==========================================
- Coverage   90.97%   90.77%   -0.21%     
==========================================
  Files         146      148       +2     
  Lines        7492     7675     +183     
  Branches     1502     1539      +37     
==========================================
+ Hits         6816     6967     +151     
- Misses        676      708      +32     

see 8 files with indirect coverage changes

@pichlermarc pichlermarc merged commit 3621e09 into open-telemetry:main Apr 18, 2024
15 checks passed
@trentm trentm deleted the tm-fix-undici-14-16-tav branch April 18, 2024 17:42
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.

2 participants