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

https support for proxy relay #564

Merged
merged 9 commits into from
Dec 2, 2024
Merged

Conversation

arenevier
Copy link
Contributor

Fix for issue #563

Copy link
Member

@jancurn jancurn left a comment

Choose a reason for hiding this comment

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

Thanks for the improvement, it looks like a reasonable change. Any chance you could also update the tests?

@arenevier
Copy link
Contributor Author

Would be happy to modify the tests. But I'm not sure what to modify. Also, running the tests hang on my laptop, after the following output:


% npm test

> [email protected] test
> nyc cross-env NODE_OPTIONS=--insecure-http-parser mocha --bail



(node:232095) Warning: Using insecure HTTP parsing
(Use `node --trace-warnings ...` to show where the warning was created)
  ✔ supports localAddress
  ✔ supports custom CONNECT server handler
  ✔ supports pre-response CONNECT payload
  utils.anonymizeProxy
    ✔ throws for invalid args
    ✔ throws for unsupported https: protocol
    ✔ throws for invalid ports
    ✔ throws for invalid URLs
    ✔ keeps already anonymous proxies (both with callbacks and promises)
    1) anonymizes authenticated upstream proxy (both with callbacks and promises)


  8 passing (63ms)
  1 failing

  1) utils.anonymizeProxy
       anonymizes authenticated upstream proxy (both with callbacks and promises):
     AssertionError: expected 'socket hang up' to include 'ECONNREFUSED'
      at /home/arno/src/proxy-chain/test/anonymize_proxy.js:236:48
      at processTicksAndRejections (node:internal/process/task_queues:95:5)

@jancurn jancurn requested a review from jirimoravcik November 20, 2024 21:50
@jirimoravcik
Copy link
Member

jirimoravcik commented Nov 21, 2024

Would be happy to modify the tests. But I'm not sure what to modify. Also, running the tests hang on my laptop, after the following output:


% npm test

> [email protected] test
> nyc cross-env NODE_OPTIONS=--insecure-http-parser mocha --bail



(node:232095) Warning: Using insecure HTTP parsing
(Use `node --trace-warnings ...` to show where the warning was created)
  ✔ supports localAddress
  ✔ supports custom CONNECT server handler
  ✔ supports pre-response CONNECT payload
  utils.anonymizeProxy
    ✔ throws for invalid args
    ✔ throws for unsupported https: protocol
    ✔ throws for invalid ports
    ✔ throws for invalid URLs
    ✔ keeps already anonymous proxies (both with callbacks and promises)
    1) anonymizes authenticated upstream proxy (both with callbacks and promises)


  8 passing (63ms)
  1 failing

  1) utils.anonymizeProxy
       anonymizes authenticated upstream proxy (both with callbacks and promises):
     AssertionError: expected 'socket hang up' to include 'ECONNREFUSED'
      at /home/arno/src/proxy-chain/test/anonymize_proxy.js:236:48
      at processTicksAndRejections (node:internal/process/task_queues:95:5)

Hey, for the tests, you can just see them run here in GitHub actions.
If you're adding a test locally, you can just run the test you've added and will (hopefully) work in CI as well.

@arenevier
Copy link
Contributor Author

I added a test. I was not able to connect to a https server in the test. But in the test, I can check that proxyServer does not consider the relay url as invalid. The code snippet will fail without the PR, and pass with it.

test/server.js Outdated Show resolved Hide resolved
test/server.js Outdated Show resolved Hide resolved
test/server.js Outdated Show resolved Hide resolved
test/server.js Outdated Show resolved Hide resolved
test/server.js Outdated Show resolved Hide resolved
test/server.js Outdated Show resolved Hide resolved
test/server.js Outdated Show resolved Hide resolved
arenevier and others added 7 commits December 2, 2024 10:24
Co-authored-by: Jiří Moravčík <[email protected]>
Co-authored-by: Jiří Moravčík <[email protected]>
Co-authored-by: Jiří Moravčík <[email protected]>
Co-authored-by: Jiří Moravčík <[email protected]>
Co-authored-by: Jiří Moravčík <[email protected]>
Co-authored-by: Jiří Moravčík <[email protected]>
@jirimoravcik jirimoravcik merged commit be4bf40 into apify:master Dec 2, 2024
4 checks passed
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.

3 participants