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

tests: fix win32 #3172

Open
wants to merge 18 commits into
base: master
Choose a base branch
from
Open

tests: fix win32 #3172

wants to merge 18 commits into from

Conversation

trim21
Copy link
Contributor

@trim21 trim21 commented Apr 17, 2024

Summary

There are several test cases broken on windows.

Checklist

  • I understand that this PR may be closed in case there was no previous discussion. (This doesn't apply to typos!)
  • I've added a test for each change that was introduced, and I tried as much as possible to make a single atomic change.
  • I've updated the documentation accordingly.

tests/test_utils.py Outdated Show resolved Hide resolved
tests/test_multipart.py Outdated Show resolved Hide resolved
@trim21
Copy link
Contributor Author

trim21 commented May 7, 2024

coverage failed because this:

httpx/httpx/_multipart.py

Lines 127 to 134 in fa6dac8

if isinstance(fileobj, io.StringIO):
raise TypeError(
"Multipart file uploads require 'io.BytesIO', not 'io.StringIO'."
)
if isinstance(fileobj, io.TextIOBase):
raise TypeError(
"Multipart file uploads must be opened in binary mode, not text mode."
)

io.StringIO is a subclass of io.TextIOBase

@trim21
Copy link
Contributor Author

trim21 commented May 7, 2024

I just use pytest's tmp_path to create a real file on disk. pytest will clean it up after test.

@trim21 trim21 requested a review from tomchristie May 7, 2024 23:18
@@ -122,6 +123,7 @@ def test_logging_redirect_chain(server, caplog):
]


@pytest.mark.skipif(sys.platform == "win32", reason="os path sep escaped on windows")
Copy link
Member

Choose a reason for hiding this comment

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

We don't need this skipif now, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we still need this skipif because path seprator / is double escaped on this tests, don't know why....

Copy link
Member

Choose a reason for hiding this comment

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

Can you remove the skipif, so we can see the exact error in CI?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

E
E         At index 1 diff: ('httpx', 10, "load_verify_locations cafile='C:\\\\Users\\\\Trim21\\\\proj\\\\httpx\\\\.venv\\\\Lib\\\\site-packages\\\\certifi\\\\cacert.pem'") != ('httpx', 10, "load_verify_locations cafile='C:\\Users\\Trim21\\proj\\httpx\\.venv\\Lib\\site-packages\\certifi\\cacert.pem'")
E
E         Full diff:
E           [
E               (
E                   'httpx',
E                   10,
E                   'load_ssl_context verify=True cert=None trust_env=True http2=False',
E               ),
E               (
E                   'httpx',
E                   10,
E                   'load_verify_locations '
E         -         "cafile='C:\\Users\\Trim21\\proj\\httpx\\.venv\\Lib\\site-packages\\certifi\\cacert.pem'",
E         +         "cafile='C:\\\\Users\\\\Trim21\\\\proj\\\\httpx\\\\.venv\\\\Lib\\\\site-packages\\\\certifi\\\\cacert.pem'",
E         ?                      ++     ++          ++    ++         ++       ++   ++                 ++       ++
E               ),
E           ]

Comment on lines +17 to +19
@pytest.mark.skipif(
sys.platform == "win32", reason="time related tests are flaky on win32"
)
Copy link
Member

Choose a reason for hiding this comment

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

I assume we could drop this, and this particular test would actually pass okay on windows?

(Also, I think we might want to raise a related refactoring issue to remove these /slow_response tests completely.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this test may pass on windows.

Copy link
Contributor Author

@trim21 trim21 May 10, 2024

Choose a reason for hiding this comment

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

it's a time accuracy problem on windows. we could increase sleep time in /slow_response to several seconds to fix this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm wrong about this, looks like increase sleep time in /slow_response won't help testing write timeout

@@ -32,3 +37,4 @@ jobs:
run: "scripts/test"
- name: "Enforce coverage"
run: "scripts/coverage"
if: "matrix.os == 'ubuntu'"
Copy link
Member

Choose a reason for hiding this comment

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

We probably don't need this conditional once we've fixed up all the tests to pass okay on windows, right?


strategy:
matrix:
os: ["ubuntu", "windows"]
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure - I'm pleased to see us fix the test cases up for supporting windows, tho do we want to double the time our test runs take to complete?

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.

2 participants