Move thread activation to start method

[samamorgan]

This moves activation of LiveServer.thread to a method. My primary motivation here is to delay thread activation so I can change the parameters of the thread in an overriden live_server fixture. This provides a work-around to issue #294 using the following override:

import os

import pytest
from django.test.testcases import _StaticFilesHandler
from pytest_django import live_server_helper
from pytest_django.lazy_django import skip_if_no_django


@pytest.fixture(scope="session")
def live_server(request):
    skip_if_no_django()

    addr = (
        request.config.getvalue("liveserver")
        or os.getenv("DJANGO_LIVE_TEST_SERVER_ADDRESS")
        or "localhost"
    )

    server = live_server_helper.LiveServer(addr)
    server.thread.static_handler = _StaticFilesHandler  # Force _StaticFilesHandler
    server.start()
    request.addfinalizer(server.stop)

    return server

[samamorgan]

Note: The failing tests here aren't failing, it's just an issue with the codecov upload.

[bluetech]

I'm not opposed to this, but needs a couple of changes:

1. To keep backward compat (not that it's a public interface, but who knows), let's add a *, start: bool = True argument to __init__, and add a call to self.start if start is true. You will then need to pass start=False.
2. The inc_thread_sharing part needs to be done in start, not in __init__, to match the stop.

[samamorgan]

I'm not opposed to this, but needs a couple of changes:

1. To keep backward compat (not that it's a public interface, but who knows), let's add a *, start: bool = True argument to __init__, and add a call to self.start if start is true. You will then need to pass start=False.
2. The inc_thread_sharing part needs to be done in start, not in __init__, to match the stop.

Thanks for the quick feedback! I did both of these, then went a little ham and took it a step beyond.

This code was a little smelly. Imports inside the constructor, messing around with threads, etc. I refactored it to move all imports to the top, and it's now subclassing LiveServerThread rather than implementing a thread internally. This is functionally the same, but cleans up a lot of unnecessary calls to self.thread/live_server.thread. The class behaves like a thread, so why not just make it one?

If I went too hard here, I'm happy to back off and just implement the requested changes.

[samamorgan]

Alright, tests failing now. Looks like I'm misunderstanding something about these imports. Is there an explanation I missed somewhere about why imports are happening in a function instead of the top of the module?

[bluetech]

importing django has some side-effects, sometimes it requires DJANGO_SETTINGS_MODULE and such to be defined, etc. So it must always be imported lazily in pytest-django.

[samamorgan]

Backed off on the subclassing decision. I spent way too much time trying to implement a lazy import mechanism just to have it fall on its face.

I very much dislike the imports inside methods. Makes my heart hurt. I don't know enough about this library to really understand what a good alternative would be though, so the code smells get to stay!

[samamorgan]

@bluetech Anything blocking this one?

[bluetech]

Suggested change

[bluetech]

I think the entire liveserver_kwargs business was to avoid this line, i.e. not duplicate Django's default in case it changes.

[bluetech]

Thanks @samamorgan. I rebased and removed unrelated changes from the diff. Merging now.