Conversation
Remove tmp_path fixture from every test, but then need to write a nested function for the fixture. Also add a DEFAULT_CONFIG for the cookiecutter generation.
matt-graham
left a comment
matt-graham
left a comment
There was a problem hiding this comment.
This looks great to me - even if it doesn't reduce line count significantly, I think the avoidance of having the same setup code across multiple tests makes the tests much more readable. I've added a couple of small suggestions and a question about how we're managing imports from helpers.py module, but these are all minor and from my perpsective this looks good to merge irrespective!
Co-authored-by: Matt Graham <matthew.m.graham@gmail.com>
for more information, see https://pre-commit.ci
Also add a default_config_with fixture for default plus one change (the main use case in our parametrised tests).
paddyroddy
left a comment
paddyroddy
left a comment
There was a problem hiding this comment.
Lines of code is a silly metric. This looks like a good change. Some comments though.
Co-authored-by: Patrick J. Roddy <patrickjamesroddy@gmail.com>
for more information, see https://pre-commit.ci
p-j-smith
left a comment
p-j-smith
left a comment
There was a problem hiding this comment.
I like this a lot, it's nice to remove some duplication. One suggestion around using indirect parameterisation
…to be used at once. Co-authored-by: Matt Graham <matthew.m.graham@gmail.com>
Contravenes PT003 but "Explicit is better than implicit." Co-authored-by: Paddy Roddy <patrickjamesroddy@gmail.com>
samcunliffe
force-pushed
the
sc/dry-up-test-fixtures
branch
from
4816a6f to
571679c
Compare
|
In the end, I think I prefer Matt's way. Though the implicit parameterisation does look very useful. |
4 participants
tmp_pathfixture from every test...default_configanddefault_config_withfixtures to help the cookiecutter generation.In the end it doesn't actually reduce the linecount (because I had to add to the docstrings).