Skip to content

Conversation

jbedard
Copy link
Contributor

@jbedard jbedard commented Jul 5, 2024

Generate the py_pytest_main target alongside tests. See #1972

@jbedard jbedard requested a review from f0rmiga as a code owner July 5, 2024 20:11
result.Imports = append(result.Imports, pyTest.PrivateAttr(config.GazelleImportsKey))
}

if len(pyTestTargets) > 0 {
Copy link
Contributor

Choose a reason for hiding this comment

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

This will make Gazelle generate a pytest main target and multiple py_test targets at the same time on the first run, and then generate another py_test target with the main pointing to the __test__ target in the second run. People will have to run Gazelle twice to reach a stable state. At this state, every test file will be consumed by two py_test targets, so bazel test //some:all will execute all tests twice.

@jbedard jbedard marked this pull request as draft July 5, 2024 21:55
},
},
{
Name: "@aspect_rules_py//py:defs.bzl",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Similarly to how we discussed the gazelle plugin being agnostic to the rule sets, it would be good to have a solution for pytest that is not specific to any specific python ruleset.

Having rules_py specific logic in a rules_python gazelle plugin sounds like our abstractions within the bazel python ecosystem are not working well enough.

We had a similar discussion about rules_pycross and rules_python targets having different naming conventions and decided to make the plugin configurable, but in this case this is overstepping that boundary and starts tailoring the gazelle plugin to very specific implementation logic.

Are there different ways to achieve the same thing?

Copy link
Contributor

Choose a reason for hiding this comment

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

We could put the plugin logic in rules_py itself, and then teach users how to compile a gazelle binary that has the plugins listed in the right order. I don't think we want to grow a Go dev-dependency if we can help it though.

Copy link
Member

Choose a reason for hiding this comment

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

We can make this file a template and allow users to pass extra stuff.

Copy link
Collaborator

@aignas aignas Jul 9, 2024

Choose a reason for hiding this comment

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

I am concerned that adding this special one off for py_pytest_main we are not making orthogonal rulesets that are easy to compose and will require continuous maintenance to tweak things. Since there are other test frameworks (like behave, robot, etc), catering for test frameworks in the gazelle plugin means that we are willing to accept contributions for all, hence I think that solving this differently would be better.

I liked the solution that @linzhp proposed in #2044. It does not add extra test framework specific concepts to the gazelle plugin and it would work with pytest_main if the users are willing to define a macro and use it, so whilst that may require users to write more code, it does mean that the rules_python project stays better scoped.

We have a maintainers meeting every Monday evening US time and we could discuss it there next week if one of you joined?

Copy link
Contributor

Choose a reason for hiding this comment

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

A modestly sophisticated user can discover the pattern suggested in #2044 to copy-paste a macro into their codebase and configure a # map_kind directive. I think that's good enough for the use cases that gazelle is trying to address.

We might do something more novice-friendly specifically in Aspect CLI.

@aignas
Copy link
Collaborator

aignas commented Jul 19, 2024

Closing in favour of #2044.

@aignas aignas closed this Jul 19, 2024
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.

5 participants