httpserver: Clear state in function-scoped fixtures before the test

[netaneld122]

I believe it makes sense to clear the server state before performing a test rather than after, starting each test in a fresh state.

Consider the following case:

@fixture
def my_fixture() -> DoesSomeHTTPRequests:
    start_thread()
    yield obj
    stop_thread()

def test_function_1(my_fixture: DoesSomeHTTPRequests, httpserver: HTTPServer) -> None:
    my_fixture.make_thread_communicate_with(httpserver.url_for('/'))

def test_function_2(httpserver: HTTPServer) -> None:
    assert len(httpserver.log) == 0

In that case - test_function_2 may receive leaked logs from test_function_1 since my_fixture is destructed only after httpserver and therefore the fixture would be able to send requests to the server after the state is cleared.

One could argue that swapping the fixtures order would solve this, but in real world scenarios fixtures can be nested and far too complex to have a well defined order for cases like that. I think that clearing the state before the test is a simple and less error prone solution.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.90%. Comparing base (0eb610b) to head (965818b).

Additional details and impacted files

@@           Coverage Diff           @@
##           master     #352   +/-   ##
=======================================
  Coverage   95.90%   95.90%           
=======================================
  Files           5        5           
  Lines         660      660           
=======================================
  Hits          633      633           
  Misses         27       27           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[csernazs]

hi @netaneld122 ,

I think your MR makes sense.
I understand the issue you descibed.

I could reproduce it without using threads:

import pytest

from pytest_httpserver import HTTPServer
import requests

class Client:
    def __init__(self):
        self.url: str | None = None

    def get(self):
        if self.url:
            requests.get(self.url)

@pytest.fixture()
def my_fixture():
    client = Client()
    yield client
    client.get()


def test_1(my_fixture: Client, httpserver: HTTPServer):
    httpserver.expect_request("/foo").respond_with_data("OK")
    my_fixture.url = httpserver.url_for("/foo")

def test_2(httpserver: HTTPServer):
    print(httpserver.log)

Basically, the issue is the order of destructing the fixtures, if httpserver gets destructed first, then it will clear its state and the remaining still-running fixtures can access it by the URL.

If I change my_fixture like this:

def my_fixture(httpserver):

Then it will define the order of destructuring (and dependency), so httpserver will be destructed later than my_fixture.

While your PR makes totally sense, I'm thinking about what could go wrong with it (assuming the existing 3rd party codes which use pytest-httpserver). I see no issues at the moment but I want to sleep on it.

Thanks,
Zsolt

[netaneld122]

Thanks for your reply @csernazs.

I only want to point out that my_fixture could be a generic fixture or similarly even a 3rd party fixture such that making it depend on httpserver would make less sense.

I understand your concerns though, perhaps bumping the minor version (rather than the patch version) could be less prone to breakage (for those who depend on ~1.0.12)?

I can also suggest clearing the state both before and after the test function, does that feel safer?

[csernazs]

Yeah, I was also thinking about clearing the state both side.

But tbh your approach seems also safe. There's no way to access the httpserver fixture without clearing it before, so I'd release your PR. Maybe 1.1.0 as you suggested.

Zsolt

[csernazs]

@netaneld122 version 1.1.0 has just been released!

Thanks for your fix!

Zsolt