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
10 changes: 8 additions & 2 deletions .github/workflows/test-suite.yml
Expand Up @@ -7,13 +7,18 @@ on:
pull_request:
branches: ["master"]

defaults:
run:
shell: bash

jobs:
tests:
name: "Python ${{ matrix.python-version }}"
runs-on: "ubuntu-latest"
name: "Python ${{ matrix.python-version }} (${{ matrix.os }})"
runs-on: "${{ matrix.os }}-latest"

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?

python-version: ["3.8", "3.9", "3.10", "3.11", "3.12"]

steps:
Expand All @@ -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?

4 changes: 3 additions & 1 deletion tests/client/test_async_client.py
Expand Up @@ -18,7 +18,9 @@ async def test_get(server):
assert response.http_version == "HTTP/1.1"
assert response.headers
assert repr(response) == "<Response [200 OK]>"
assert response.elapsed > timedelta(seconds=0)
# time related tests are flaky on windows
# Sanity test the property access and return type.
assert response.elapsed >= timedelta(seconds=0)


@pytest.mark.parametrize(
Expand Down
12 changes: 9 additions & 3 deletions tests/test_multipart.py
Expand Up @@ -3,6 +3,7 @@
import io
import tempfile
import typing
from pathlib import Path

import pytest

Expand Down Expand Up @@ -371,10 +372,15 @@ def test_multipart_encode_files_raises_exception_with_StringIO_content() -> None
httpx.Request("POST", url, data={}, files=files) # type: ignore


def test_multipart_encode_files_raises_exception_with_text_mode_file() -> None:
def test_multipart_encode_files_raises_exception_with_text_mode_file(
tmp_path: Path,
) -> None:
url = "https://www.example.com"
with tempfile.TemporaryFile(mode="w") as upload:
files = {"file": ("test.txt", upload, "text/plain")}
# TemporaryFiles are always binary mode on windows.
# For this test case where we need a text-mode file.
tmp_path.joinpath("test.txt").write_text("content")
with tmp_path.joinpath("test.txt").open() as temp:
files = {"file": ("test.txt", temp, "text/plain")}
with pytest.raises(TypeError):
httpx.Request("POST", url, data={}, files=files) # type: ignore

Expand Down
5 changes: 5 additions & 0 deletions tests/test_timeouts.py
@@ -1,3 +1,5 @@
import sys

import pytest

import httpx
Expand All @@ -12,6 +14,9 @@ async def test_read_timeout(server):
await client.get(server.url.copy_with(path="/slow_response"))


@pytest.mark.skipif(
sys.platform == "win32", reason="time related tests are flaky on win32"
)
Comment on lines +17 to +19
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.

@pytest.mark.anyio
async def test_write_timeout(server):
timeout = httpx.Timeout(None, write=1e-6)
Expand Down
10 changes: 8 additions & 2 deletions tests/test_utils.py
Expand Up @@ -2,6 +2,7 @@
import logging
import os
import random
import sys

import certifi
import pytest
Expand Down Expand Up @@ -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           ]

def test_logging_ssl(caplog):
caplog.set_level(logging.DEBUG)
with httpx.Client():
Expand Down Expand Up @@ -155,7 +157,9 @@ def test_get_ssl_cert_file():
os.environ["SSL_CERT_FILE"] = str(TESTS_DIR / "test_utils.py")
# SSL_CERT_FILE is correctly set, SSL_CERT_DIR is not set.
ca_bundle = get_ca_bundle_from_env()
assert ca_bundle is not None and ca_bundle.endswith("tests/test_utils.py")
assert ca_bundle is not None and ca_bundle.endswith(
os.path.join("tests", "test_utils.py")
)

os.environ["SSL_CERT_FILE"] = "wrongfile"
# SSL_CERT_FILE is set with wrong file, SSL_CERT_DIR is not set.
Expand All @@ -170,7 +174,9 @@ def test_get_ssl_cert_file():
os.environ["SSL_CERT_FILE"] = str(TESTS_DIR / "test_utils.py")
# Two environments is correctly set.
ca_bundle = get_ca_bundle_from_env()
assert ca_bundle is not None and ca_bundle.endswith("tests/test_utils.py")
assert ca_bundle is not None and ca_bundle.endswith(
os.path.join("tests", "test_utils.py")
)

os.environ["SSL_CERT_FILE"] = "wrongfile"
# Two environments is set but SSL_CERT_FILE is not a file.
Expand Down