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
Add support for extra env variables in go tests #16013
Conversation
[ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]
This seemed pretty straightforward (thanks for the very helpful hints @Eric-Arellano!), but since it's my first PR I went through the WIP/Draft workflow. Broke the change up so that hopefully it's easier to review the two additions. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Woohoo! Thanks so much! I'll cherry-pick this to 2.13 because it seems low risk and we haven't yet switched from a1
to rc0
Hmm. Looks like I broke the protobuf tests. I'll see if I can fix that. |
@@ -179,6 +180,18 @@ class SkipGoTestsField(BoolField): | |||
help = "If true, don't run this package's tests." | |||
|
|||
|
|||
class GoTestExtraEnvVarsField(StringSequenceField): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should PythonTestsExtraEnvVarsField
and this class share a common base class? The help message would be in the base class. This subclass can override the alias
to add that test_
prefix.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can do that if that's preferred. Is there an example of something similar?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PrefixedJvmResolveField
and JvmResolveField
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is fine
Looks like rule graph errors. If the rules modified by this PR now depend on rules in another file, the rules in that other file will need to be referenced in the tests' |
[ci skip-rust] [ci skip-build-wheels]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Sorry for the delay, I was out sick with Covid
@@ -179,6 +180,18 @@ class SkipGoTestsField(BoolField): | |||
help = "If true, don't run this package's tests." | |||
|
|||
|
|||
class GoTestExtraEnvVarsField(StringSequenceField): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is fine
This addresses pantsbuild#15963 by plumbing through `[test].extra_env_vars` and adding the `test_extra_env_vars` field to `go_package`. [ci skip-rust] [ci skip-build-wheels]
This addresses #15963 by plumbing through
[test].extra_env_vars
and adding thetest_extra_env_vars
field togo_package
.[ci skip-rust]
[ci skip-build-wheels]