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: Handle dotted parameters in classname #200

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 8 additions & 2 deletions src/syrupy/location.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@
Optional,
)

from pytest import Class


class TestLocation:
def __init__(self, node: Any):
Expand All @@ -17,8 +19,12 @@ def __init__(self, node: Any):

@property
def classname(self) -> Optional[str]:
_, __, qualname = self._node.location
return ".".join(list(self.__valid_ids(qualname))[:-1]) or None
return (
".".join(
node.name for node in self._node.listchain() if isinstance(node, Class)
)
or None
)
Comment on lines +22 to +27
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would prefer we fixed the __valid_ids method so both __parse and classname are using the same logic

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I suspect classname doesn't need that logic – unlike with the others, in this context, we know that we have some set of valid Python identifiers anyway. It seems like it might, then, be slightly better to actually look for classes in the node hierarchy, as per this implementation?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm missed this yesterday, it's an easy enough refactor to revert. @noahnu what do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, I mean, that's what this implementation does – listchain shows all the current "nodes" in the test chain (modules, classes, &c.), so I'm going off that directly.

The alternative is just to split on [ and ignoring the part after that in __valid_ids before splitting on ., which gets you to the same place.


@property
def filename(self) -> str:
Expand Down
3 changes: 3 additions & 0 deletions stubs/pytest.pyi
Original file line number Diff line number Diff line change
Expand Up @@ -3,3 +3,6 @@ from typing import Any, Callable, TypeVar
ReturnType = TypeVar("ReturnType")

def fixture(func: Callable[..., ReturnType]) -> Callable[..., ReturnType]: ...

class Class:
name: str
9 changes: 9 additions & 0 deletions tests/__snapshots__/test_extension_amber.ambr
Original file line number Diff line number Diff line change
Expand Up @@ -155,6 +155,12 @@
],
}
---
# name: test_doubly_parametrized[bar-foo]
'foo'
---
# name: test_doubly_parametrized[bar-foo].1
'bar'
---
# name: test_empty_snapshot
None
---
Expand Down Expand Up @@ -216,6 +222,9 @@
# name: test_numbers.2
0.3333333333333333
---
# name: test_parameter_with_dot[value.with.dot]
'value.with.dot'
---
# name: test_reflection
SnapshotAssertion(name='snapshot', num_executions=0)
---
Expand Down
12 changes: 12 additions & 0 deletions tests/test_extension_amber.py
Original file line number Diff line number Diff line change
Expand Up @@ -169,3 +169,15 @@ def test_nested_class_method(self, snapshot, actual):

class TestSubClass(TestClass):
pass


@pytest.mark.parametrize("parameter_with_dot", ("value.with.dot",))
def test_parameter_with_dot(parameter_with_dot, snapshot):
assert parameter_with_dot == snapshot


@pytest.mark.parametrize("parameter_1", ("foo",))
@pytest.mark.parametrize("parameter_2", ("bar",))
def test_doubly_parametrized(parameter_1, parameter_2, snapshot):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was about to implement something that just split on [, on closer inspection, it seemed better to traverse the node's parents.

assert parameter_1 == snapshot
assert parameter_2 == snapshot