Replace _get_url abstract test method with a pytest fixture#751
Conversation
|
@AdrianDAlessandro I needed to implement this to do tests where we needed to pass url params to the Also there was a failing test on the main branch. I'm not sure if this is just local to me. |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
AdrianDAlessandro
left a comment
AdrianDAlessandro
left a comment
There was a problem hiding this comment.
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.
| def _get_url(self, **kwargs): | ||
| return reverse("skill_detail", kwargs=kwargs) |
There was a problem hiding this comment.
Is what you are trying to do achieved by this existing pattern?
| def test_template_used(self, admin_client, url): | ||
| """Test the correct template is used by the GET request.""" | ||
| with assertTemplateUsed(template_name=self._template_name): | ||
| response = admin_client.get(self._get_url(slug=skill.slug)) |
There was a problem hiding this comment.
Results in being able to pass args to reverse like this
The existing method works if you are calling 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
left a comment
AdrianDAlessandro
left a comment
There was a problem hiding this comment.
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?
| class TestSkillPageView404: | ||
| """Tests related to the 404 error for non-existent skills.""" | ||
|
|
||
| @pytest.fixture | ||
| def url(self): | ||
| """Fixture to get the test url for a non-existent skill.""" | ||
| return reverse("skill_detail", kwargs={"slug": "nonexistent-skill"}) |
There was a problem hiding this comment.
I'm not a fan of needing to create a whole new class just for testing a different query parameter for the same url. I think the point of these classes and mixing is to be able to group all the relevant testing for each base url into the one class.
| def soup(self, client, url) -> BeautifulSoup: | ||
| """A fixture of the BeautifulSoup4 object of the requested page.""" | ||
| response = client.get(self._get_url()) | ||
| response = client.get(url) | ||
| return BeautifulSoup(response.content, "html.parser") |
There was a problem hiding this comment.
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_soupWhich 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()2 participants
Description
Replaces the use of the
_get_urlabstract method with adef url(self)...fixture. This allows accessing other fixture data such as database mocked instances.Fixes #750
Type of change
Key checklist
python -m pytest)mkdocs serve)pre-commit run --all-files)Further checks