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

go_test: Add env attribute #3004

Merged
merged 2 commits into from
Nov 30, 2021
Merged

Conversation

illicitonion
Copy link
Contributor

@illicitonion illicitonion commented Nov 12, 2021

What type of PR is this?

Feature

What does this PR do? Why is it needed?

This allows setting environment variables before static initialisers
run, and which reference expanded locations.

Which issues(s) does this PR fix?

Partially fixes #2921

@google-cla google-cla bot added the cla: yes label Nov 12, 2021
@illicitonion illicitonion force-pushed the test-env branch 9 times, most recently from 79e1d9e to 20e980e Compare November 19, 2021 12:25
Previously it was not possible to look up runfiles from a manifest
unless they were in the main workspace. Factor in in-path workspaces
when looking up runfiles.
@illicitonion illicitonion force-pushed the test-env branch 2 times, most recently from c0ffbfd to 49df7be Compare November 19, 2021 13:39
@illicitonion
Copy link
Contributor Author

I think this is ready for review :)

/cc @linzhp @robfig

@linzhp linzhp requested a review from blico November 19, 2021 16:58
@blico
Copy link
Contributor

blico commented Nov 22, 2021

Thanks for working on this @illicitonion.

My understanding is that we could add support for this attribute using the TestEnvironment provider avoiding the need for a wrapper executable around go_test. https://docs.bazel.build/versions/main/skylark/lib/testing.html#TestEnvironment.

With this approach, we should just need to run whatever pre-processing is needed on the env attr (e.g. ctx.expand_location) and then construct a TestEnvironment provider and return it through go_test.

It would also be good to add some documentation to the go_test rule about the new attr, and some test cases verifying env is available during TestMain and the contents of env are expanded.

This allows setting environment variables before static initialisers
run, and which reference expanded locations.

See bazel-contrib#2921 for more context.
@illicitonion
Copy link
Contributor Author

My understanding is that we could add support for this attribute using the TestEnvironment provider avoiding the need for a wrapper executable around go_test. https://docs.bazel.build/versions/main/skylark/lib/testing.html#TestEnvironment.

Oh nice, that's much simpler! Done!

It would also be good to add some documentation to the go_test rule about the new attr

Added to core.rst

and some test cases verifying env is available during TestMain and the contents of env are expanded.

There's one test (tests/core/go_test/env_test.go) showing that an env var is set and expanded to the path of a file which can then be read - are there more contexts you have in mind to test?

@blico blico changed the title Add env attribute to go_test go_test: Add env attribute Nov 30, 2021
@blico blico merged commit 94ce014 into bazel-contrib:master Nov 30, 2021
@illicitonion illicitonion deleted the test-env branch November 30, 2021 17:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

no such attribute 'env' in 'go_transition_test' rule
2 participants