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

fix: unused snapshot detection for targeting single parameterized test case #394

Merged
merged 18 commits into from
Oct 30, 2020

Conversation

iamogbz
Copy link
Collaborator

@iamogbz iamogbz commented Oct 27, 2020

Description

Fix unused snapshot detection for targeting single parameterized test case.
Clarify test methods for integration tests and testing in general.

Additionally

  • Fix issue where snapshot location can be __snapshot__/test_file. where extension is an empty string.

Related Issues

Checklist

  • This PR has sufficient documentation.
  • This PR has sufficient test coverage.
  • This PR title satisfies semantic convention.

Additional Comments

No additional comments.

@codecov
Copy link

codecov bot commented Oct 27, 2020

Codecov Report

Merging #394 into master will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##            master      #394   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           19        17    -2     
  Lines         1002       992   -10     
=========================================
- Hits          1002       992   -10     

Comment on lines -114 to +164
if any(
TestLocation(node).matches_snapshot_name(snapshot.name)
for node in self.ran_items
)
if self._selected_items_match_name(snapshot.name)
or self._provided_nodes_match_name(snapshot.name, provided_nodes)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is the main gist of the fix. Basically _selected_items_match_name use the keyword expressions when they are present or matches against the ran items when no keywords provided or _provided_nodes_match_name checks if the nodes given for the test file matching this fossil location matches the snapshot name.

Comment on lines -92 to +137
return self.all_items == self.ran_items and not self.is_providing_nodes
return self.all_items == self.ran_items
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Moved the and not self.is_providing_nodes check to the point of usage at if self.ran_all_collected_tests and not any(provided_nodes):, makes the method name more accurate.

maybe_opt_arg = None

for maybe_opt_arg, arg_value in arg_groups:
if maybe_opt_arg == "-k": # or maybe_opt_arg == "-m":
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ignoring the marker expression as they can not be used to match against snapshot names and are already used to select which pytest items in the collection to run.

)

import attr
import pytest
from _pytest.mark.expression import Expression
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Alternative to importing the protected class is to roll our own parsing of -k expressions.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Just confirming this works in the min. version of pytest we support?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Haven't will test that out, probably should have a way to automate the CI range test

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah no this does not work with pytest 5...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Changed this to include a naive version of the Expression class for compatibility with older pytest versions while still giving us some of the scaffolding for keyword snapshot inclusion

assert Path(*snapshot_path, "test_updated_1.ambr").exists()


def test_update_targets_only_selected_parametrized_tests_for_update_dash_k(
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This test was updated to catch the case raised by this issue

@iamogbz iamogbz requested a review from noahnu October 29, 2020 22:43
@iamogbz iamogbz marked this pull request as ready for review October 29, 2020 23:36
src/syrupy/report.py Outdated Show resolved Hide resolved
@iamogbz iamogbz requested a review from noahnu October 30, 2020 02:12
Copy link
Collaborator

@noahnu noahnu left a comment

Choose a reason for hiding this comment

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

LGTM

@iamogbz iamogbz merged commit e008935 into master Oct 30, 2020
@iamogbz iamogbz deleted the fix-test-matching branch October 30, 2020 02:57
syrupy-bot pushed a commit that referenced this pull request Oct 30, 2020
## [0.8.2](v0.8.1...v0.8.2) (2020-10-30)

### Bug Fixes

* unused snapshot detection for targeting single parameterized test case ([#394](#394)) ([e008935](e008935))
@syrupy-bot
Copy link
Contributor

🎉 This PR is included in version 0.8.2 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

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