Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add information from tests to ward fixtures #161

Merged
merged 20 commits into from Jul 10, 2020
Merged

Add information from tests to ward fixtures #161

merged 20 commits into from Jul 10, 2020

Conversation

JoshKarpel
Copy link
Contributor

So, this turned into a doozy. I think it's ready for a round of review.

Addresses #159

  • Test and Fixture are now hashable. This was a little tricky for Fixture, since it seems like the underlying function's hash is different in different places (I suspect this is because they aren't actually the same function when inside the Suite because of assertion rewriting?).
  • Test now has a helper class, TestArgumentResolver, where are all of the methods that make that happen are stored. I actually didn't really need to modify them at all, although I changed how information flows through them a little and added a helper method to get fixture arguments. This made the rest of the logic a little easier for me to grok and provides an obvious place to hang more code in the future. More data could be passed to the resolver directly to lessen the amount of self.test.<whatever> going on in it.
  • ward fixtures can now display three kinds of dependency information: parent fixtures, child fixtures, and direct test usages. Parents and children can be displayed as trees.
  • Rewrote most of the fixture information display from Add CLI command to show fixture information #154 to accommodate the new functionality.

@darrenburns
Copy link
Owner

Thanks for this, and sorry for the delay. Will hopefully be able to give a first review this week.

@darrenburns
Copy link
Owner

An initial finding after checking out the branch and running ward fixtures --show-dependency-trees:

Screenshot 2020-05-11 at 16 51 16

These fixtures are actually used by tests, but the tests themselves are parameterised.

@using(
    root_file=each("pyproject.toml", ".git"),
    project=each(fake_project_pyproject, fake_project_git),
)
@test("find_project_root finds project root with '{root_file}' file")
def _(root_file, project):
    root = find_project_root([project / "a/b/c", project / "a/d"])
    assert root.resolve() == project.resolve()
    assert (root / root_file).exists() == True

ward/terminal.py Outdated Show resolved Hide resolved
@JoshKarpel
Copy link
Contributor Author

Ok, I think I got the parameterisation down. I introduced a new layer in the argument resolver, _get_args_for_iteration, which you can call to resolve each but not fixtures. _resolve_args uses _get_args_for_iteration internally, which means there are two passes over the test arguments.

One thing I noticed is that the search-like things still only effect test collection, not fixture collection, because I'm comparing against the defined fixtures. This may produce pretty unexpected results, because you put in a search, and it'll suddenly tell you that a bunch of your fixtures from modules that you may have explicitly excluded from the search are unused.

Thoughts on how to approach that? Applying the same criteria to both doesn't quite seem right. My instinct is that the fixtures command should always look at all tests, and the searches should apply to the fixtures, not the tests. (I think you may have indicated that that was your desired behavior in #154 but it went over my head at the time.)

@darrenburns
Copy link
Owner

I agree with your instincts: I think the search should just apply to fixtures instead of tests. ward fixtures should probably collect all tests. As a result, I think we could drop --search from ward fixtures, since fixtures don't have full string descriptions and so the value of searching drops considerably?

I think it's still useful to be able to limit by paths though, where the fixtures listed are only those that lie within the specified paths. e.g. show me the usage of all fixtures defined in my ./integration_tests directory: ward fixtures --path integration_tests.

It does raise another question as to how --path would work in pyproject.toml. When you add path=something to pyproject.toml, it'd apply to both ward test and ward fixtures as it stands. So if --path works for both those subcommands, a single item in the config would alter the defaults for 2 subcommands in slightly different ways. My initial thought is that each subcommand could have it's own section of the config. Right now we have [tool.ward], but there could be [tool.ward.fixtures] and so on.

I'll try and have a look at the latest changes over the weekend, thanks! :)

@JoshKarpel JoshKarpel marked this pull request as draft May 16, 2020 15:40
@JoshKarpel
Copy link
Contributor Author

So, I took the coward's way out and used a new argument -f, --fixture-path instead of re-using --path. However, I think this is probably the right way to do it. Fixture discovery inherently depends on test discovery: we find fixtures by implicitly assuming that if they are used in a test we discovered, they must have been defined at some point. So you need separate control over what ward imports and what fixtures it ends up displaying. The fact that you get the intersection here is a little brain-melting, but I think it makes sense that --path is a more "global" option.

--search for fixtures looks in function and module names, as well as function bodies. I suspect this is actually the more useful filtering option, since you might be looking at a test and already know the fixture name, but not where it's defined. Maybe it should also search in docstrings? Given that tests have descriptions and tags it doesn't seem worth searching in their docstrings, but since fixtures have neither of those, it might make sense for them. I think it's unlikely that people will set search in their config.

That being said, I think it makes a lot of sense to have a config subsection for each subcommand.

Idle thought: what if fixtures could have tags as well, and those tags would be propagated to their descendants? A slow fixture marks all of the tests that depend on it as slow, etc.

@JoshKarpel JoshKarpel marked this pull request as ready for review May 23, 2020 23:29
Copy link
Owner

@darrenburns darrenburns left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good! I've just requested a few small changes in my review.

ward/terminal.py Outdated Show resolved Hide resolved
ward/testing.py Outdated Show resolved Hide resolved
ward/util.py Show resolved Hide resolved
ward/testing.py Outdated Show resolved Hide resolved
ward/testing.py Show resolved Hide resolved
ward/terminal.py Outdated Show resolved Hide resolved
Co-authored-by: Darren Burns <darrenburns@users.noreply.github.com>
@codecov-commenter
Copy link

codecov-commenter commented Jun 4, 2020

Codecov Report

Merging #161 into master will increase coverage by 0.33%.
The diff coverage is 73.07%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #161      +/-   ##
==========================================
+ Coverage   72.94%   73.27%   +0.33%     
==========================================
  Files          16       16              
  Lines        1349     1452     +103     
  Branches      243      263      +20     
==========================================
+ Hits          984     1064      +80     
- Misses        318      341      +23     
  Partials       47       47              
Impacted Files Coverage Δ
ward/terminal.py 41.54% <20.33%> (-1.81%) ⬇️
ward/run.py 92.20% <71.42%> (-2.32%) ⬇️
ward/collect.py 79.59% <100.00%> (+2.31%) ⬆️
ward/fixtures.py 97.08% <100.00%> (+1.62%) ⬆️
ward/testing.py 92.79% <100.00%> (+0.66%) ⬆️
ward/util.py 83.33% <100.00%> (+6.06%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a77c137...dfbd899. Read the comment docs.

Co-authored-by: Darren Burns <darrenburns@users.noreply.github.com>
JoshKarpel and others added 3 commits June 4, 2020 09:41
Co-authored-by: Darren Burns <darrenburns@users.noreply.github.com>
# Conflicts:
#	ward/tests/test_fixtures.py
#	ward/tests/test_testing.py
Copy link
Owner

@darrenburns darrenburns left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for making those changes, and sorry for the wait! 🏖️

@darrenburns darrenburns merged commit 67ff1bb into darrenburns:master Jul 10, 2020
@JoshKarpel JoshKarpel deleted the more-fixtures branch July 10, 2020 15:39
@JoshKarpel
Copy link
Contributor Author

No worries! 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ward fixtures highlighting fixtures that aren't injected into any tests
3 participants