feat(gazelle): Add "python_test_file_pattern" directive #1819

dougthor42 · 2024-03-22T03:32:37Z

Add the python_test_file_pattern directive. This directive allows
users to configure what python files get mapped to the py_test
rule.

The default behavior is unchanged: both test_* and *_test.py
files generate py_test targets if the directive is unset.

The directive supports multiple glob patterns, separated by a comma.

Note: The original code used, effectively, test_* for one of the
patterns. This code uses test_*.py instead. These are equivalent
because of the .py extension check prior to pattern matching.

Fixes #1816.

Add glob to generate.go Add glob dep

gazelle/python/generate.go

gazelle/python/testdata/directive_python_test_file_pattern/README.md

aignas · 2024-03-24T07:45:12Z

gazelle/python/generate.go

+func makeCompiledGlobs(globs []string) []glob.Glob {
+	compiledGlobs := []glob.Glob{}
+	for _, value := range globs {
+		compiledGlob := glob.MustCompile(value)


I might prefer a more helpful error message if the glob fails. Could we add an integration test? Maybe it is good enough already? Right now this code would panic

Failure to compile: error message added, test added

I had a similar thought last night: we also need to handle the case where the user has # gazelle:python_test_file_pattern (with no value). Right now no files will be mapped to py_test if there is no pattern. If we want to keep that behavior, I'll need to update the docs.

gazelle/python/generate.go

gazelle/python/testdata/directive_python_test_file_pattern/README.md

dougthor42

I also added a test that subpackages can override the value.

dougthor42 · 2024-03-24T20:29:47Z

gazelle/python/generate.go

+func makeCompiledGlobs(globs []string) []glob.Glob {
+	compiledGlobs := []glob.Glob{}
+	for _, value := range globs {
+		compiledGlob := glob.MustCompile(value)


Failure to compile: error message added, test added

I had a similar thought last night: we also need to handle the case where the user has # gazelle:python_test_file_pattern (with no value). Right now no files will be mapped to py_test if there is no pattern. If we want to keep that behavior, I'll need to update the docs.

dougthor42 · 2024-03-24T20:35:15Z

gazelle/python/configure.go

+		case pythonconfig.TestFilePattern:
+			globStrings := strings.Split(strings.TrimSpace(d.Value), ",")
+			config.SetTestFilePattern(globStrings)


In #1787 I added an option for resetting the directive to the rules_python default. Do we want to support the same for this directive?

// N.B.: pseudocode; haven't tested yet case pythonconfig.TestFilePattern: directiveArg := strings.TrimSpace(d.Value) if directiveArg == "DEFAULT" { directiveArg = pythonconfig.DefaultTestFilePatternString } globStrings := strings.Split(directiveArg, ",") config.SetTestFilePattern(globStrings)

For consistency I would agree, but at the same time my brain is also telling me that we could have:

gazelle:python_test_file_pattern

to reset the config to defaults.

The empty pattern yielding no test targets is not super intuitive, IMHO.

What do you think?

The empty pattern yielding no test targets is not super intuitive, IMHO.

Agreed.

That said, I would argue that the empty pattern resetting to defaults is also not super intuitive. Some readers will see that and think "no test targets" while others will think "resets to default".

I like explicit things 😁. There's no ambiguity and it's easier to grok when returning to something N years down the road.

If we wanted to support yielding no test targets¹ it should be something like gazelle:python_test_file_pattern NONE or have a different directive altogether (gazelle:python_no_tests or something).

Footnotes

I can't think of a reason why someone would want that, but that doesn't mean there aren't reasons ↩

Passerby opinion: yielding no test targets can (and imho should) be achieved by ignoring files with gazelle:python_ignore_files. That said, setting pattern to NONE should not be allowed (also, it's a bit confusing to set pattern to NONE, some might thing there is NONE file or smths). gazelle:python_test_file_pattern (blank) should also fail.

yielding no test targets can (and imho should) be achieved by ignoring files with gazelle:python_ignore_files

I disagree. There are definitely cases where you want to yield no test targets but do want to yield lib and binary targets.

project/ + BUILD.bazel + MODULE.bazel + src/myfoo/ + BUILD.bazel + foo.py + foo_test.py + subdir/ + BUILD.bazel + run_test.py # py_binary + example_test.py # py_library

Using gazelle:python_ignore_files would mean that no targets are generated in subdir above, when really we'd want only py_library and py_binary targets made.

There's also the case where test files are outside of the package, as described by PyPA Tutorial. In that case, you can be sure that all targets in src/mypackage below are py_library and py_binary.

project/ + BUILD.bazel + MODULE.bazel + src/mypackage/ # guaranteed to have no py_test targets + BUILD.bazel + run_electrical_test.py # py_binary + electrical_test.py # py_library + tests/ # any py_* targets + BUILD.bazel + foo_test.py # py_test + bar_test.py # py_test + test_utils.py # py_library + run_all_test.py # py_binary

also, it's a bit confusing to set pattern to NONE, some might thing there is NONE file

A valid point. Perhaps using something that is unlikely to be a filename would be better? Maybe one of these or similar?

# gazelle:python_test_file_pattern %NONE% # gazelle:python_test_file_pattern !!NO_FILES # gazelle:python_test_file_pattern ~!~NO_PATTERN~!~

One issue is that linux filenames can be basically anything haha so it's not feasible to pick a string that can't be a filename.

If we do go with a more "special" string, I'd probably want to update the python_default_visibility directive special strings to match.

gazelle:python_test_file_pattern (blank) should also fail.

+1 to that.

Agreed with arguments around gazelle:python_ignore_files.

How about...not having any magic names at all? I mean, by the time someone wants to reset the test file patterns, he/she already learned what default values are (while customising them in the first place), and if resetting the patterns is needed, it can be done with # gazelle:python_test_file_pattern *_test,test_*.

Yet another idea is to have dedicated no-args directive for resetting to default, something like:
# gazelle:default_python_test_file_pattern. However, it only makes sense if applied conventionally everywhere in the plugin imho. Otherwise, I'd vote for not having any special values and let users "reset" by explicitly setting python_test_file_pattern to default values.

I'm good with no magic names at all and a "reset" by explicitly setting the value to defaults. gazelle:python_test_file_pattern *_test.py,test_*.py

That said, I'd still like to mention the two (minor) issues with no magic values:

[can ignore] There's no way to defer to rules_python's defaults. For example, if rules_python changes the default from *_test.py,test_*.py to something else, users would have to update their configs manually if they wanted to always use the rules_python default.

TBH this is probably a good thing because it results in less unexpected behavior.

We're back to the # gazelle:python_test_file_pattern (no value) for preventing any files from mapping to py_test. As mentioned previously:

this is not very intuitive.

we said that "no value" should fail.

For (2), perhaps just making sure that's documented and tested is sufficient? Unless we implement another directive gazelle:python_no_test_files or something (which I'm against - it seems to only complicate things).

have dedicated no-args directive for resetting to default ... it only makes sense if applied conventionally everywhere in the plugin

+1. I'm slightly unsure about having directives to reset to default (as you said, people can just set the default manually), but if we add them they definitely should be applied everywhere.

As far as I can see, the plugin currently has no switches for enabling/disabling parts of the generation process. Setting pattern to none sounds to me like an abuse of the directive to disable tests and I'd rather have an explicit python_no_test_files so that there is a clear indication no tests are being processed for a) confusing fewer humans, and b) plugin itself, so that it can potentially deviate its processing depending on that fact (e.g. print debug/warning message etc).

This should be, however, out of context for this PR. I'd say, we should just disallow no-arg python_test_file_pattern, provide no magic values and wrap-up the PR.

Have I already mentioned this is great work and nice improvement 🙂? Thank you! Looking forward to it. I have exactly the place where I will use it already.

Setting pattern to none sounds to me like an abuse of the directive

I can see that and it's a good point.

I'd rather have an explicit python_no_test_files so that there is a clear indication no tests are being processed ... we should just disallow no-arg python_test_file_pattern, provide no magic values and wrap-up the PR.

SGTM. I'd like to continue the discussion, so I've opened #1826.

this is great work and nice improvement

Thanks! I've been really enjoying being able to add more features.

All this SGTM, thanks for the discussion. I've been enjoying the read! :)

dougthor42

I've added the test case for "no value causes error" and updated the docs for this directive. I think we're ready for re-review.

gazelle/README.md

dougthor42 · 2024-03-28T17:46:36Z

gazelle/go.mod

@@ -9,6 +9,7 @@ require (
 	github.com/bmatcuk/doublestar v1.3.4
 	github.com/emirpasic/gods v1.18.1
 	github.com/ghodss/yaml v1.0.0
+	github.com/gobwas/glob v0.2.3


This seems to be the most popular glob package, though I don't see any github activity in 6 years. Other options were:

https://pkg.go.dev/github.com/ryanuber/go-glob

https://pkg.go.dev/v.io/v23/glob

How about doublestar.Match? It is used by bazel-gazelle and is already included in this go.mod:9 too.

import ( "fmt" "github.com/bmatcuk/doublestar/v4" ) func matchesAnyGlob(s string, globs []string) bool { for _, g := range globs { if ok, _ := doublestar.Match(g, s); ok { return true } } return false } func main() { defaultGlob := []string{"*_test.py", "test_*.py"} fmt.Println(matchesAnyGlob("test_my_target.py", defaultGlob)) fmt.Println(matchesAnyGlob("my_target_test.py", defaultGlob)) customGlob := []string{"*_pytest.py", "pytest_*.py"} fmt.Println(matchesAnyGlob("pytest_upload.py", customGlob)) fmt.Println(matchesAnyGlob("download_pytest.py", customGlob)) }

+1 for not adding another dep! Updated.

dougthor42

removed gobwas/glob and migrated/upgraded to doublestar/v4.

dougthor42 · 2024-04-03T02:56:04Z

gazelle/go.mod

@@ -9,6 +9,7 @@ require (
 	github.com/bmatcuk/doublestar v1.3.4
 	github.com/emirpasic/gods v1.18.1
 	github.com/ghodss/yaml v1.0.0
+	github.com/gobwas/glob v0.2.3


+1 for not adding another dep! Updated.

wingsofovnia

Thank you for the great work!

aignas · 2024-04-03T10:56:50Z

gazelle/python/generate.go

@@ -54,6 +54,19 @@ func GetActualKindName(kind string, args language.GenerateArgs) string {
 	return kind
 }

+func matchesAnyGlob(s string, globs []string) bool {
+	for _, g := range globs {
+		ok, err := doublestar.Match(g, s)


I am slightly concerned that we are compiling the glob here. Would it be possible to compile it elsewhere? When parsing the config would be another option.

Given that this is used in larger repos I am worried that we perform a linear number of glob compilations as opposed to doing it once when loading the config.

Agreed. We were doing that before¹ with gobwas/glob. From what I can tell, doublestar doesn't have any kind of "compiled pattern" object that I can pass around instead.

We can validate patterns during config load, but we wouldn't be able to tell doublestart.Match to not validate patterns and thus save some cycles. (Unless there's a way to access matchWithSeparator? My understanding of go is that lowercase names aren't exported and are thus not accessible.)

I think we have two (three?) options:

Go back to gobwas/glob and precompile patterns

Accept the perf hit.

Some go ability that I'm not aware of?

Either way we should validate/compile patterns when loading the config so that we fail earlier. I've made that update. It means we validate one more time, but we're already validating (N_files * Patterns) times so (N*P)+1 is no big deal

Footnotes

Well, almost. It wasn't done when loading config, but at least it was outside of the for _, f := range args.RegularFiles loop. ↩

Looking at the match's code there is nothing to "compile" in glob really, like we typically do with regex. This is common for globs to be matched directly as they are simple, do not need complicated parsing, creating underlying tree used for matching etc.

This [rules_python] repo already does doublestart.Match in a loop here and so does bazel-gazelle here and it hasn't been an issue so far.

Validating earlier is a good improvement.

Unless there's a way to access matchWithSeparator? My understanding of go is that lowercase names aren't exported and are thus not accessible.

That is correct for go. I created a ticket bmatcuk/doublestar#92

…well, in addition to) during generate.GenerateRules

…test

dougthor42 · 2024-04-03T16:50:31Z

gazelle/python/generate.go

@@ -54,6 +54,19 @@ func GetActualKindName(kind string, args language.GenerateArgs) string {
 	return kind
 }

+func matchesAnyGlob(s string, globs []string) bool {
+	for _, g := range globs {
+		ok, err := doublestar.Match(g, s)


Agreed. We were doing that before¹ with gobwas/glob. From what I can tell, doublestar doesn't have any kind of "compiled pattern" object that I can pass around instead.

We can validate patterns during config load, but we wouldn't be able to tell doublestart.Match to not validate patterns and thus save some cycles. (Unless there's a way to access matchWithSeparator? My understanding of go is that lowercase names aren't exported and are thus not accessible.)

I think we have two (three?) options:

Go back to gobwas/glob and precompile patterns

Accept the perf hit.

Some go ability that I'm not aware of?

Either way we should validate/compile patterns when loading the config so that we fail earlier. I've made that update. It means we validate one more time, but we're already validating (N_files * Patterns) times so (N*P)+1 is no big deal

Footnotes

Well, almost. It wasn't done when loading config, but at least it was outside of the for _, f := range args.RegularFiles loop. ↩

dougthor42 · 2024-04-03T17:19:06Z

gazelle/pythonconfig/pythonconfig.go

+func (c *Config) SetTestFilePattern(patterns []string) {
+	for _, p := range patterns {
+		if !doublestar.ValidatePattern(p) {
+			log.Fatalf("ERROR: Failed to compile glob '%v'. Error: syntax error in pattern\n", p)


globstar errors with path.ErrBadPAttern but also states "users should not count on the returned error, doublestar.ErrBadPattern, being equal to path.ErrBadPattern" so I've opted to inject the string manually.

Actually... I could just do:

// We only care if the pattern is valid. Use `Match` instead of `ValidatePattern` (which // just returns `bool`) so that we always use the error message provided by `doublestar`. if _, err := doublestar.Match(p, ""); err != nil { log.Fatalf("ERROR: Failed to compile glob '%v'. Error: %v\n", p, err) } ...

Any preference between
(a) using ValidatePattern and manually injecting the error string as I've already done or
(b) abusing Match?

I guess the word "abusing" in your comment speaks for itself :) Looking at the Match's code, err.ErrBadPattern brings no more than just an error flag (no error message, no extra context etc.) so it is effectively does the same as having true/false. I'd keep it as is. Except maybe a nit regarding go error message style (see the comment in configure.go).

Suggested change

log.Fatalf("ERROR: Failed to compile glob '%v'. Error: syntax error in pattern\n", p)

log.Fatalf("invalid glob pattern '%s'", p)

👍 kept, aside from style nit

dougthor42 · 2024-04-03T17:25:11Z

gazelle/pythonconfig/pythonconfig.go

+// Will exit 1 if the pattern is invalid.
+func (c *Config) SetTestFilePattern(patterns []string) {
+	for _, p := range patterns {
+		if !doublestar.ValidatePattern(p) {


Would this check be better in configure.go case pythonconfig.TestFilePattern:?

I'd say it would be slightly more aligned with existing code to do to in a case indeed as there are already some invalid value errors and parsing there while these stay simple setters.

👍 moved.

aignas · 2024-04-04T09:10:24Z

Ah, thanks for checking. Then I am happy to merge everything! :)

…

On 4 April 2024 17:38:05 GMT+09:00, Illia Ovchynnikov ***@***.***> wrote: @wingsofovnia commented on this pull request. > @@ -54,6 +54,19 @@ func GetActualKindName(kind string, args language.GenerateArgs) string { return kind } +func matchesAnyGlob(s string, globs []string) bool { + for _, g := range globs { + ok, err := doublestar.Match(g, s) Looking at the match's [code](https://github.com/bmatcuk/doublestar/blob/180028ba525d3116cc917ac3179e79428acfd89c/match.go#L74) there is nothing to "compile" in glob really, like we typically do with regex. This repo already does `double start.Match` in the loop [here](https://github.com/bazelbuild/rules_python/blob/b9f39bf09760ff75aedd64ce99c9573c1a97068f/gazelle/python/generate.go#L188) and so does bazel-gazelle [here](https://github.com/bazelbuild/bazel-gazelle/blob/1b331b6d9cb2d981580a8d3e2dfc551185790737/walk/config.go#L145) and it hasn't been an issue so far. -- Reply to this email directly or view it on GitHub: #1819 (comment) You are receiving this because you modified the open/close state. Message ID: ***@***.***>

wingsofovnia · 2024-04-04T09:12:38Z

gazelle/python/configure.go

+		case pythonconfig.TestFilePattern:
+			value := strings.TrimSpace(d.Value)
+			if value == "" {
+				log.Fatal("ERROR: Directive 'python_test_file_pattern' requires a value.")


nit: there is no need to double severity in the message here as Fatal will be already in the log. Also, go's error messages are conventionally lowercased since errors can be wrapped/nested (e.g. something bad happened: directive 'python...). \n is also unnecessary.

Suggested change

log.Fatal("ERROR: Directive 'python_test_file_pattern' requires a value.")

log.Fatal("directive 'python_test_file_pattern' requires a value.")

(See https://go.dev/wiki/CodeReviewComments#error-strings)

TIL, thanks! Updated.

As an FYI, the ERROR: and \n were included to maintain consistency with the other log.Fatal messages:

$ rg 'log.Fatal.?\("' manifest/generate/generate.go 90: log.Fatalf("ERROR: %v\n", err) 109: log.Fatalf("ERROR: %v\n", err) pythonconfig/pythonconfig.go 444: log.Fatalf("ERROR: Failed to compile glob '%v'. Error: syntax error in pattern\n", p) python/lifecycle.go 41: log.Fatalf("failed to write parser zip: %v", err) 47: log.Fatalf("cannot write %q: %v", helperPath, err) python/generate.go 238: log.Fatalf("ERROR: %v\n", err) 319: log.Fatalf("ERROR: %v\n", err) 353: log.Fatalf("ERROR: %v\n", err) 384: log.Fatalf("ERROR: %v\n", err) 504: log.Fatalf("ERROR: %v\n", err) python/configure.go 188: log.Fatal("ERROR: Directive 'python_test_file_pattern' requires a value.")

Prior to this PR, 7 of 9 calls included both ERROR: and \n.

dougthor42 added 9 commits March 21, 2024 15:19

Update CHANGELOG

056c4e4

Fix README

ac364d2

Add glob to go.mod and go.sum

a852679

Add glob to generate.go Add glob dep

add test cases

d4b456b

Add 'python_test_file_pattern' directive

5cbcd67

Add docs

abe667c

Forgot a docstring :-P

98922c2

How'd a MODULE.bazel sneak in there?

f9897b0

:set spell

a3cc524

dougthor42 commented Mar 22, 2024

View reviewed changes

gazelle/python/generate.go Outdated Show resolved Hide resolved

gazelle/python/testdata/directive_python_test_file_pattern/README.md Show resolved Hide resolved

dougthor42 marked this pull request as ready for review March 22, 2024 03:57

dougthor42 requested review from f0rmiga and rickeylev as code owners March 22, 2024 03:57

aignas reviewed Mar 24, 2024

View reviewed changes

dougthor42 added 3 commits March 24, 2024 13:55

Add test for bad globs

cf274c3

Handle invalid glob patterns nicely.

a156e2e

Add test case for sub-packages overriding parent values.

f1b66a2

dougthor42 commented Mar 24, 2024

View reviewed changes

dougthor42 mentioned this pull request Mar 28, 2024

[Continuing Discussion] python_no_test_files directive #1826

Open

dougthor42 added 5 commits March 28, 2024 10:21

Fail for no value

9eb66f4

Update docs

0f2b9aa

sync default in docs

7b7648b

Don't log.Fatalf when regular log.Fatal will do

fd21bc9

Update testdata README

ca79dfa

dougthor42 commented Mar 28, 2024

View reviewed changes

dougthor42 requested review from wingsofovnia and aignas March 28, 2024 17:54

dougthor42 added 3 commits April 2, 2024 20:53

Remove 'glob' dependency

17e14d7

Update to doublestar v4

19b9a49

Use doublestar.Match instead

5875c2f

dougthor42 commented Apr 3, 2024

View reviewed changes

wingsofovnia approved these changes Apr 3, 2024

View reviewed changes

aignas approved these changes Apr 3, 2024

View reviewed changes

aignas added this pull request to the merge queue Apr 3, 2024

aignas removed this pull request from the merge queue due to a manual request Apr 3, 2024

aignas requested changes Apr 3, 2024

View reviewed changes

dougthor42 added 3 commits April 3, 2024 10:08

Validate patterns during pythonconfig.SetTestFilePattern instead of (…

df8f967

…well, in addition to) during generate.GenerateRules

clarify comment

b14b0dd

Now that we validate patterns early, we don't need a .py file in the …

d3a5364

…test

dougthor42 commented Apr 3, 2024

View reviewed changes

wingsofovnia reviewed Apr 4, 2024

View reviewed changes

dougthor42 added 3 commits April 4, 2024 09:37

Error message sytle fixes

988ae8c

Move pattern validation from pythonconfig.go to generate.go

4d41e94

Merge branch 'main' into test-file-pattern-gh1816

58d5571

dougthor42 requested a review from aignas April 5, 2024 20:54

aignas approved these changes Apr 5, 2024

View reviewed changes

aignas added this pull request to the merge queue Apr 5, 2024

Merged via the queue into bazelbuild:main with commit 5667a86 Apr 5, 2024
4 checks passed

dougthor42 deleted the test-file-pattern-gh1816 branch April 6, 2024 02:34

wingsofovnia mentioned this pull request Jun 11, 2024

Gazelle: Sanitize distribution names in modules mapping / manifest, not at runtime #1939

Closed

	log.Fatalf("ERROR: Failed to compile glob '%v'. Error: syntax error in pattern\n", p)
	log.Fatalf("invalid glob pattern '%s'", p)

	log.Fatal("ERROR: Directive 'python_test_file_pattern' requires a value.")
	log.Fatal("directive 'python_test_file_pattern' requires a value.")

feat(gazelle): Add "python_test_file_pattern" directive #1819

feat(gazelle): Add "python_test_file_pattern" directive #1819

Conversation

dougthor42 commented Mar 22, 2024 • edited Loading

Choose a reason for hiding this comment

dougthor42 Mar 24, 2024 • edited Loading

Choose a reason for hiding this comment

dougthor42 left a comment

Choose a reason for hiding this comment

dougthor42 Mar 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Footnotes

wingsofovnia Mar 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wingsofovnia Mar 27, 2024 • edited Loading

Choose a reason for hiding this comment

dougthor42 Mar 27, 2024 • edited Loading

Choose a reason for hiding this comment

wingsofovnia Mar 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dougthor42 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dougthor42 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wingsofovnia left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Footnotes

wingsofovnia Apr 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Footnotes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wingsofovnia Apr 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wingsofovnia Apr 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aignas commented Apr 4, 2024 via email

wingsofovnia Apr 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dougthor42 commented Mar 22, 2024 •

edited

Loading

dougthor42 Mar 24, 2024 •

edited

Loading

dougthor42 Mar 24, 2024 •

edited

Loading

wingsofovnia Mar 26, 2024 •

edited

Loading

wingsofovnia Mar 27, 2024 •

edited

Loading

dougthor42 Mar 27, 2024 •

edited

Loading

wingsofovnia Mar 28, 2024 •

edited

Loading

wingsofovnia Apr 4, 2024 •

edited

Loading

wingsofovnia Apr 4, 2024 •

edited

Loading

wingsofovnia Apr 4, 2024 •

edited

Loading

wingsofovnia Apr 4, 2024 •

edited

Loading