Replace _get_url abstract test method with a pytest fixture

[sbland]

• fixes Use url fixture instead of abstract method in tests #750

Description

Replaces the use of the _get_url abstract method with a def url(self)... fixture. This allows accessing other fixture data such as database mocked instances.

Fixes #750

Type of change

• Documentation (non-breaking change that adds or improves the documentation)
• New feature (non-breaking change which adds functionality)
• Optimization (non-breaking, back-end change that speeds up the code)
• Technical work (non-breaking, change which is work as part of a new feature)
• Bug fix (non-breaking change which fixes an issue)
• Breaking change (whatever its nature)

Key checklist

• All tests pass (eg. python -m pytest)
• The documentation builds and looks OK (eg. mkdocs serve)
• Pre-commit hooks run successfully (eg. pre-commit run --all-files)

Further checks

• Code is commented, particularly in hard-to-understand areas
• Tests added or an issue has been opened to tackle that in the future. (Indicate issue here: # (issue))

[sbland]

@AdrianDAlessandro I needed to implement this to do tests where we needed to pass url params to the _get_url method, particularly for beautiful soup tests.

Also there was a failing test on the main branch. I'm not sure if this is just local to me.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

[AdrianDAlessandro]

I'm not sure exactly what this is trying to fix. The issue says it's to be able to pass arguments to the _get_url method, but that is already done in some places. I've commented on the parts that do it.

[AdrianDAlessandro]

Is what you are trying to do achieved by this existing pattern?

[AdrianDAlessandro]

Results in being able to pass args to reverse like this

[sbland]

I'm not sure exactly what this is trying to fix. The issue says it's to be able to pass arguments to the _get_url method, but that is already done in some places. I've commented on the parts that do it.

The existing method works if you are calling _get_url from inside the class that defines get_url but doesn't work when we use _get_url in a fixture. Example here: https://github.com/direct-framework/direct-webapp/blob/main/tests/main/view_utils.py#L63

The use case I couldn't implement is when the url should use params that are defined from mocked database instance objects.

Edit:

class BS4Mixin(ABC):
    """Mixin for tests that use BeautifulSoup4. It makes the `soup` fixture available.

    Note: Using this requires the test class to define:
        - A `_get_url` method
    """

    @abstractmethod
    def _get_url(self) -> str:
        return NotImplemented

    @pytest.fixture
    def soup(self, client) -> BeautifulSoup:
        """A fixture of the BeautifulSoup4 object of the requested page."""
        response = client.get(self._get_url()) # << This does not allow passing args
        return BeautifulSoup(response.content, "html.parser")
...

[AdrianDAlessandro]

I don't love this approach because it breaks the intention behind the current approach. I have suggested an alternative in the comment below. What do you think @sbland? Does that also solve the problem?

[AdrianDAlessandro]

This still results in not being able to pass arguments to client.get without create a whole new class with a different url fixture. Which would be the same as creating a whole new class with a different _get_url method. And (as mentioned in another comment), I'm not a fan of needing to create separate classes for all of the edge cases.

I think maybe the best solution is to change these soup fixtures (because I think they're the only problematic ones) into factory fixtures.

For example (requires a from collections.abc import Callable at the top):

    @pytest.fixture
    def soup_factory(self, client) -> Callable[..., BeautifulSoup]:
        """A fixture factory for the BeautifulSoup4 object of the requested page."""

        def get_soup(**kwargs) -> BeautifulSoup:
            if kwargs:
                response = client.get(self._get_url(kwargs=kwargs))
            else:
                response = client.get(self._get_url())
            return BeautifulSoup(response.content, "html.parser")

        return get_soup

Which can then be called as a function inside the test using it. There could also be a default one that is using it without arguments, to keep the current behaviours consistent:

    @pytest.fixture
    def soup(self, soup_factory) -> BeautifulSoup:
        """A fixture of the BeautifulSoup4 object of the requested page."""
        return soup_factory()

[sbland]

I had a go at implementing this and it works great for BS but not for the TemplatMixin that calls get_url from a mixin test.

class TemplateOkMixin(ABC):
    """Mixin for tests that verify the correct template usage.

    Note: Using this requires the test class to define:
        - A `_get_url` method
        - A `_template_name` variable
    """

    _template_name: str

    def test_template_used(self, admin_client):
        """Test the correct template is used by the GET request."""
        with assertTemplateUsed(template_name=self._template_name):
            response = admin_client.get(self._get_url())
        assert response.status_code == HTTPStatus.OK

[AdrianDAlessandro]

What's the specific problem? The BeautifulSoup one is specifically to handle a fixture named soup, but this Mixin is just for including this test into a class without needing to manually re-write it

The purpose of using this mixin is just to include that one check that the correct template is used. It's not the most essential one to have. Maybe if a page is more complicated and always requires a parameter to be sent to self._get_url then this Mixin shouldn't be used? But I'm curious about the specific issue first.

[sbland]

@AdrianDAlessandro I think the best solution here is to just not use the mixin like you said. I had another look and there is no way to pass an arg to this without going back to the complicated method above.

[sbland]

@AdrianDAlessandro I've updated the implementation and added tests for skill profile page to show it works.

I think we can perhaps close this one now.

[AdrianDAlessandro]

This approach looks good, just fix those failing tests and it'll be ready