octokit / core.js

Extendable client for GitHub's REST & GraphQL APIs
MIT License
1.19k stars 310 forks source link

test(agent-proxy): restore test coverage with `undici` #587

Closed oscard0m closed 1 year ago

oscard0m commented 1 year ago

Resolves #586


Behavior

Before the change?

After the change?

Other information


Additional info

Pull request checklist

Does this introduce a breaking change?

Please see our docs on breaking changes to help!

If Yes, what's the impact:

Pull request type

Type: Maintenance


oscard0m commented 1 year ago

Before getting deeper into the debugging, does someone with experience with undici's fetch and the dispatcher option guide me on what I could be missing?

When running the npm run test, my debuggers are not stopping in the proxy handler. The first layers in the stack of calls seem that the dispatcher is arriving to undici correctly.

Tagging @nickfloyd @wolfy1339 @gr2m (I think we are trying to solve something similar here) ^1^3

gr2m commented 1 year ago

It only seems to be a problem with the proxy test, the certificate tests, passes, right?

I can only reproduce the error in the proxy test, the certificate test passes

➜  core.js git:(restore-test-coverage-for-agent-proxy-with-undici) npx jest test/agent-proxy/agent-proxy-test.test.ts
 FAIL  test/agent-proxy/agent-proxy-test.test.ts
  client proxy
    ✕ options.request.fetch = customFetch with dispatcher: new ProxyAgent(proxyUrl) (25 ms)

  ● client proxy › options.request.fetch = customFetch with dispatcher: new ProxyAgent(proxyUrl)

    expect(received).toBe(expected) // Object.is equality

    Expected: true
    Received: false

      61 |     await octokit.request("/");
      62 |
    > 63 |     expect(proxyReceivedRequest).toBe(true);
         |                                  ^
      64 |     expect.assertions(4);
      65 |   });
      66 | });

      at Object.<anonymous> (test/agent-proxy/agent-proxy-test.test.ts:63:34)

Test Suites: 1 failed, 1 total
Tests:       1 failed, 1 total
Snapshots:   0 total
Time:        1.023 s, estimated 4 s

I looked into it for a bit but couldn't figure out what's wrong 😞 What I'd do next is to do a undici-only test setup that uses a proxy, i'm sure there are examples out there, worst case look at unidici's test for the dispatcher feature

oscard0m commented 1 year ago

@gr2m I would like you to look into this when you have time, considering you got deeper with undici and so.

oscard0m commented 1 year ago

@gr2m I would like you to look into this when you have time, considering you got deeper with undici and so.

@gr2m I'm merging this but feel free to comment and I can open a follow-up PR.

oscard0m commented 1 year ago

Changes look great 👍🏼 I've been on vacation last week, sorry for the late response

No worries! I hope you enjoyed them! Don't hesitate to comment / ask and I will be happy to open a follow-up PR

github-actions[bot] commented 1 year ago

:tada: This PR is included in version 5.0.1 :tada:

The release is available on:

Your semantic-release bot :package::rocket: