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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

[tests] Add tests for socks proxies #7908

Merged
merged 13 commits into from Aug 25, 2023

Conversation

coletdjnz
Copy link
Member

@coletdjnz coletdjnz commented Aug 19, 2023

This PR implements some long needed test cases for our socks proxy module. Previously, only a few e2e tests were implemented and were disabled by default.

As part of this I have written basic socks4/5 dummy proxy servers (as per the specs) that allow us to get back proxy debug information to test against.

Instead of testing the socks.py module directly, I am testing at the handler level. This is to allow us to test socks proxy implementations in multiple handlers. Additionally, the tests are written to work with different protocols other than http (for when we need to implement websockets over socks, for instance).

When writing these tests, the following bugs/issues with our socks client implementation have been found:

Tests have been written for these cases but are currently skipped until they are fixed. I plan to look into these in followup PRs.

Template

Before submitting a pull request make sure you have:

In order to be accepted and merged into yt-dlp each piece of code must be in public domain or released under Unlicense. Check all of the following options that apply:

  • I am the original author of this code and I am willing to release it under Unlicense
  • I am not the original author of this code but it is in public domain or released under Unlicense (provide reliable evidence)

What is the purpose of your pull request?

Copilot Summary

馃 Generated by Copilot at 37438eb

Summary

馃搫馃敡馃И

Refactor networking tests and add a handler fixture. This change improves the code structure and reusability of the tests for different networking backends.

Pytest fixture of doom, testing the handler of the beast
Refactoring the imports, breaking the chains of the code
Conftest.py is the file, where the magic happens
Networking backends of fire, burning the bridges of the web

Walkthrough

  • Add a new pytest fixture handler in conftest.py that returns a partial function of a request handler class based on the parameter passed to the test function (link)
  • Use the handler fixture to test different networking backends for yt-dlp, such as urllib, pycurl, etc. (link, link, link)
  • Set a fake logger for the request handler to avoid printing messages to the console during testing (link)
  • Move some of the imports from test_networking.py to conftest.py since they are only used by the handler fixture and not by the test functions (link)
  • Remove the import of _REQUEST_HANDLERS from test_networking.py since it is no longer needed there (link)
  • Remove the definition of the handler fixture from test_networking.py since it is now moved to conftest.py (link)

Copy link
Member

@Grub4K Grub4K left a comment

Choose a reason for hiding this comment

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

Cosmetic things mainly

test/test_socks.py Outdated Show resolved Hide resolved
test/test_socks.py Outdated Show resolved Hide resolved
test/test_socks.py Show resolved Hide resolved
test/test_socks.py Show resolved Hide resolved
test/test_socks.py Outdated Show resolved Hide resolved
test/test_socks.py Show resolved Hide resolved
test/test_socks.py Show resolved Hide resolved
test/test_socks.py Show resolved Hide resolved
@coletdjnz coletdjnz merged commit fcd6a76 into yt-dlp:master Aug 25, 2023
13 checks passed
@coletdjnz coletdjnz deleted the feature/networking/socks-tests branch August 25, 2023 07:10
aalsuwaidi pushed a commit to aalsuwaidi/yt-dlp that referenced this pull request Apr 21, 2024
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

2 participants