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

lookup: re-add undici #982

Merged
merged 1 commit into from
Mar 7, 2024
Merged

lookup: re-add undici #982

merged 1 commit into from
Mar 7, 2024

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

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
Copy link
Member

mcollina commented Oct 4, 2023

@mcollina
Copy link
Member

@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
Copy link
Member

@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

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

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

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

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.

@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
8 checks passed
@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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants