Skip to content

lookup: re-add undici#982

Merged
targos merged 1 commit intomainfrom
re-add-undici
Mar 7, 2024
Merged

lookup: re-add undici#982
targos merged 1 commit intomainfrom
re-add-undici

Conversation

@Trott
Copy link
Member

@Trott Trott commented Sep 24, 2023

Checklist
  • npm test passes
  • contribution guidelines followed
    here

@codecov-commenter
Copy link

codecov-commenter commented Sep 24, 2023 •
edited
Loading

Codecov Report

All modified lines are covered by tests ✅

Comparison is base (f15c9a2) 96.44% compared to head (4211ffe) 96.44%.
Report is 12 commits behind head on main.

Additional details and impacted files 
@@           Coverage Diff           @@
##             main     #982   +/-   ##
=======================================
  Coverage   96.44%   96.44%           
=======================================
  Files          28       28           
  Lines        2139     2139           
=======================================
  Hits         2063     2063           
  Misses         76       76

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mcollina
Copy link
Member

mcollina commented Sep 28, 2023

CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/3273/

@mcollina
Copy link
Member

mcollina commented Oct 4, 2023

Fresh CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/3303/

@mcollina
Copy link
Member

mcollina commented Oct 19, 2023

Fresh CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/3334/

@targos
Copy link
Member

targos commented Oct 19, 2023

Failure from https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/3334/nodes=rhel8-x64/

 # Subtest: test/connect-timeout.js
     # Subtest: priotorise socket errors over timeouts
         1..1
         not ok 1 - should be equal
           ---
           compare: ===
           at:
             line: 16
             column: 9
             file: test/connect-timeout.js
           stack: |
             test/connect-timeout.js:16:9
           source: |2
                 .catch((err) => {
                   t.equal(err.code, 'ENOTFOUND')
             --------^
                 })
           diff: |
             --- expected
             +++ actual
             @@ -1,1 +1,1 @@
             -ENOTFOUND
             +UND_ERR_CONNECT_TIMEOUT
           ...
         
         # failed 1 test
     not ok 1 - priotorise socket errors over timeouts # time=1074.305ms
     
     # Subtest: connect-timeout
         1..1
         ok 1 - type is ConnectTimeoutError
     ok 2 - connect-timeout # time=1003.865ms
     
     # Subtest: connect-timeout
         1..1
         ok 1 - type is ConnectTimeoutError
     ok 3 - connect-timeout # time=1003.412ms
     
     1..3
     # failed 1 of 3 tests
     # time=5100.208ms
 not ok 30 - test/connect-timeout.js # time=5100.208ms
   ---
   env: {}
   file: test/connect-timeout.js
   timeout: 60000
   command: /home/iojs/build/workspace/citgm-smoker/smoker/bin/node
   args:
     - --expose-gc
     - test/connect-timeout.js
   stdio:
     - 0
     - pipe
     - 2
   cwd: /home/iojs/tmp/citgm_tmp/1031071a-c640-4205-a5f9-5946076938c2/undici
   exitCode: 1
   ...

@mcollina mcollina mentioned this pull request Oct 19, 2023
Merged
@mcollina
Copy link
Member

mcollina commented Oct 23, 2023

@nodejs/undici I need some help in making the tests less flaky so we can re-land this.

@ronag
Copy link
Member

ronag commented Oct 23, 2023

I think you fixed connect-timeout? What tests are still flaky?

@mcollina
Copy link
Member

mcollina commented Oct 23, 2023

Let's try again with v5.26.5.

Fresh CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/3337/

@RafaelGSS
Copy link
Member

RafaelGSS commented Jan 16, 2024

Full CITGM for undici https://ci.nodejs.org/job/citgm-smoker/3383/

(I'm not sure if I'm configuring it correctly)

@targos
Copy link
Member

targos commented Jan 16, 2024

@RafaelGSS You used the wrong job. It should be https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker-pipeline/

@mcollina
Copy link
Member

mcollina commented Jan 16, 2024

Note this will fail due to the regression in Node v21.6.0

This fixes it but I'm not sure it's the right fix: nodejs/undici#2617

@panva
Copy link
Member

panva commented Jan 16, 2024 •
edited
Loading

https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker-pipeline/237/

(v18 and v20)

The undici test suite appears too flaky to be in citgm in its current state.

@panva panva mentioned this pull request Jan 16, 2024
Closed
@KhafraDev KhafraDev mentioned this pull request Jan 23, 2024
Closed
6 tasks
@targos
Copy link
Member

targos commented Mar 7, 2024

We need to revisit this. Another regression appeared in v21.7.0 and would have been caught by undici.

@mcollina
Copy link
Member

mcollina commented Mar 7, 2024

IMHO this should never have been removed in the first place (I said as much multiple times). Let's land this even if it fails and refine.

@targos targos merged commit 38c15c2 into main Mar 7, 2024
@targos targos deleted the re-add-undici branch March 7, 2024 09:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ljharb ljharb ljharb approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@Trott @codecov-commenter @mcollina @targos @ronag @RafaelGSS @panva @ljharb