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

support pytest python_functions config via a pytest plugin #157

Merged
merged 7 commits into from
Jan 28, 2024

Conversation

nitsanavni
Copy link
Contributor

@nitsanavni nitsanavni commented Jan 9, 2024

fixes #155 "on my machine"

TODO

  • add tests

@nitsanavni
Copy link
Contributor Author

@emilybache please review

return is_unittest_test or is_pytest_test
return StackFrameNamer.is_unittest_test(
frame
) or StackFrameNamer.is_pytest_test(method_name)
Copy link
Contributor

Choose a reason for hiding this comment

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

it's bothering me that the StackFrameNamer.is_unittest_test and is_pytest_test take different arguments. I'd pass the frame to both.

# Check that name looks like a glob-string before calling fnmatch
# because this is called for every name in each collected module,
# and fnmatch is somewhat expensive to call.
elif ("*" in pattern or "?" in pattern or "[" in pattern) and fnmatch.fnmatch(
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this the same code that pytest uses internally? Is there any way to call the same code in pytest as a library?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed, good eye.
See discussion here.

@@ -0,0 +1,4 @@
from .pytest_config import set_pytest_config

def pytest_configure(config):
Copy link
Contributor

Choose a reason for hiding this comment

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

I wasnt sure how this was all wired together - what calls pytest_configure?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a pytest plugin, I think pytest treats any function starting with pytest_ in a plugin module as an entry point to the plugin and sends the config as a fixture.

@emilybache
Copy link
Contributor

I'm a little concerned that this code might solve the immediate problem but not in a sustainable way. It seems to be duplicating functionality in pytest, by reading pytest's configuration file. I wonder if there is a way to re-use the configuration file parsing code from pytest itself?

@nitsanavni
Copy link
Contributor Author

@emilybache Thanks for reviewing!

duplicating functionality in pytest

Indeed it does, but does not rely on the implementation but rather on the public pytest configuration interface.
I've considered calling pytest internals, but that would make our code depend on implementation details rather than on public interfaces which tend to be more stable. (For example I've noticed that the specific required code has moved between different files and classes between different pytest versions)

Copying the code was an easy way to avoid rewriting it, but since there's no obvious way to DRY it I was ok with keeping it as is. I do think that this code merits some unit testing on our side, though.

The parsing of the pytest.ini file is still done by pytest, not us. We merely use the plugin mechanism (which is also a public interface of pytest) to access the specific config field of interest.

@emilybache
Copy link
Contributor

That is a good response to my concern. It's good to be relying on a public pytest interface. The amount of duplicated code is small and hopefully relatively stable since it refers to the wildcards used in teh published format of the pytest.ini file.

nitsanavni and others added 3 commits January 14, 2024 20:34
Co-Authored-By: 4dsherwood <[email protected]>
Co-Authored-By: Nitsan Avni <[email protected]>
Co-Authored-By: blade290 <[email protected]>
Co-Authored-By: 4dsherwood <[email protected]>
Co-Authored-By: Nitsan Avni <[email protected]>
@emilybache
Copy link
Contributor

looks good to me!

@nitsanavni nitsanavni changed the title support pytest python functions via a pytest plugin support pytest python_functions config via a pytest plugin Jan 15, 2024
@JulieGunawan
Copy link
Contributor

JulieGunawan commented Jan 21, 2024

Potential starting point for writing test:
https://github.com/nitsanavni/ApprovalTests.Python/tree/test-example/tests/PythonTest

@isidore isidore merged commit 97de123 into approvals:main Jan 28, 2024
24 checks passed
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.

request to support pytest configured with other naming conventions other than "test_"
4 participants