Uh oh!
There was an error while loading. Please reload this page.
Add a pytest rules - #464
Conversation
thundergolfer
left a comment
There was a problem hiding this comment.
Thanks for kicking this off. Will be taking it for a spin in https://github.com/thundergolfer-playground/rules_python-pr-464
| }, | ||
| ) | ||
| def pytest_test(name, srcs, deps = None, args = None, data = None, python_version = None, **kwargs): |
There was a problem hiding this comment.
py_pytest_test, like Dropbox uses? I like that it preserves the standard py_ prefix.
| load("@pip//:requirements.bzl", "requirement") | ||
| TEST_DEPS = [ | ||
| requirement("pluggy"), |
There was a problem hiding this comment.
Where are these deps coming from? The relevant requirements.txt file is empty in the PR.
| import sys | ||
| import pytest | ||
| args = ["-ra"] + %s + sys.argv[1:] + %s |
There was a problem hiding this comment.
Was going to request --long-args for readability but seems it's just -r chars provided by pytest, lame.
| tests.append(test_name) | ||
| pytest_test( |
There was a problem hiding this comment.
Don't think we should have the 'generic py_test_suite delegate to a pytest test implementation. I think everything using pytest (which is a third-party dep and not stock python) should include "pytest" in the rule name.
| ) | ||
| native.test_suite( | ||
| name = name, | ||
| tests = tests, |
There was a problem hiding this comment.
If the
testsattribute is unspecified or empty, the rule will default to including all test rules in the current BUILD file that are not tagged as manual. source
?? That's unexpected behaviour to me. I would have thought an empty list would do nothing or error. Do you have experience with _test_suite rules that wrap native.test_suite? Do rule authors keep this odd behaviour?
| name = name, | ||
| python_version = python_version, | ||
| srcs = srcs + [runner_target], | ||
| deps = deps, |
There was a problem hiding this comment.
Think that users of the any _pytest_test rule should get pytest as a provided dependency, like here: https://github.com/dropbox/dbx_build_tools/blob/fe5c9e668a9e6951970c0595089742d8a0247b8c/build_tools/py/py.bzl#L1011
Is there an problem with that approach I'm not seeing?
There was a problem hiding this comment.
Only issue I can think of involves which version of pytest to use. Presumably users need to have it in their requirements.txt, or we inject a default version if not. We should definitely add requirement("pytest") to deps if it doesn't exist.
Other related issues include user provided plugin version eg. pytest_coverage which need to resolve with pytest, and how / when to add those deps to the py_pytest_test rule.
| def pytest_test(name, srcs, deps = None, args = None, data = None, python_version = None, **kwargs): | ||
| runner_target = "%s-runner.py" % name | ||
| _pytest_runner( |
There was a problem hiding this comment.
What necessitates the creation of a separate executable? Can we just have a special py_test.main program? It looks like that's maybe what's happening as the py_test.main value is main = runner_target. The _pytest_runner target creates the runner file. But then why is the _pytest_runneritself executable?
joshua-cannon-techlabs
commented
Sep 1, 2021
Firstly, I would absolutely love to see However, one thing that worries me is capturing the invisible dependency between test files and So if the user is specifying the test, someone (is it them or the framework?) should be declaring those dependencies ( The same goes for the test suit rule. |
hrfuller
left a comment
There was a problem hiding this comment.
I'd love to revive this PR. As a first pass we can't and shouldn't aim to support all the configurability of pytest, but we should let users passthrough pytest args to the stub script that invokes it.
Other things to consider are:
- How to treat configuration files we find in the test sandbox, easiest thing at first would be to ignore custom configs.
- we should amend the
srcsof the underlying native.py_test to include a glob for
**/conftest.py as someone mentioned in a comment.
| expanded_args = [ctx.expand_location(arg, ctx.attr.data) for arg in ctx.attr.args] | ||
| runner = ctx.actions.declare_file(ctx.attr.name) | ||
| ctx.actions.write( |
There was a problem hiding this comment.
This should be a template substitution action.
| name = name, | ||
| python_version = python_version, | ||
| srcs = srcs + [runner_target], | ||
| deps = deps, |
There was a problem hiding this comment.
Only issue I can think of involves which version of pytest to use. Presumably users need to have it in their requirements.txt, or we inject a default version if not. We should definitely add requirement("pytest") to deps if it doesn't exist.
Other related issues include user provided plugin version eg. pytest_coverage which need to resolve with pytest, and how / when to add those deps to the py_pytest_test rule.
aignas
commented
Dec 28, 2021
When someone comes back to this, consider adding extra arguments to change how pytest is changing the import path. I did some investigation as to why pytest tests where failing when I was using protobuf rules here: https://github.com/aignas/bazel_pytest_proto |
@AutomatedTester are you planning/willing to pick this up again, or would you prefer someone else taking it over? @everyone else in here: what would be a minimal setup of changes required here to get this merged? i see the following todo-list:
i would argue we shouldn't aim for adressing all eventualities at first, otherwise this would never get in in any form. just for clarification: i would be willing to take this over under the condition that there is a willingness for constructive cooperation and eventually compromise. |
AutomatedTester
commented
May 23, 2022
Happy for someone to take this over, I haven't had time to do it. |
thundergolfer
commented
Jun 22, 2022
Heads up that #723 may supersede this. |
This Pull Request has been automatically marked as stale because it has not had any activity for 180 days. It will be closed if no further activity occurs in 30 days. |
This PR was automatically closed because it went 30 days without a reply since it was labeled "Can Close?" |
alexeagle
commented
Jul 31, 2023
Note, https://docs.aspect.build/rules/aspect_rules_py/docs/rules#py_pytest_main provides missing glue for pytest, thanks to @mattem and @f0rmiga |
PR Checklist
Please check if your PR fulfills the following requirements:
.parfiles. See CONTRIBUTING.md for infoPR Type
What kind of change does this PR introduce?
What is the current behavior?
Issue Number: N/A
What is the new behavior?
Does this PR introduce a breaking change?
Other information