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

pass exec_properties to underlying android_local_test macro #810

Merged
merged 3 commits into from
Aug 5, 2022

Conversation

Augustyniak
Copy link
Contributor

@Augustyniak Augustyniak commented Aug 3, 2022

exec_properties is supported by android_local_test, it should be supported by kt_android_local_test too.

@google-cla
Copy link

google-cla bot commented Aug 3, 2022

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@@ -119,4 +120,5 @@ def kt_android_local_test(
manifest_values = manifest_values,
tags = kwargs.get("tags", default = None),
testonly = testonly,
exec_properties = exec_properties,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Wouldn't we want to also pass this to the rest of the targets that are created here so that the exec_properties are consistent for all targets?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense 👍 I think that your comment should be addressed in 9c922b5

Copy link
Collaborator

Choose a reason for hiding this comment

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

https://github.com/bazelbuild/rules_kotlin/pull/810/files#diff-686482fb7b1546f7ba4e615a0e5f45354cf1f74934c3a31165571540ff96796dR68 the kt_android_library macro could use this property as well so that the two macros provide consistent attributes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, this time it should really be addressed in 9601771

@Augustyniak Augustyniak force-pushed the pass-exec-properties branch from 2441fb7 to 38258d2 Compare August 3, 2022 23:24
Signed-off-by: Rafal Augustyniak <[email protected]>
@Bencodes Bencodes merged commit f9d2b55 into bazelbuild:master Aug 5, 2022
Augustyniak added a commit to envoyproxy/envoy-mobile that referenced this pull request Aug 17, 2022
Description: Add support for Kotlin robolectric tests by making `envoy_mobile_android_test` macro use `kt_android_local_test` maco as opposed to `android_local_test` macro as the former supports both Java and Kotlin files while the latter supports Java files only. The change should be additive when it comes to functionality. Bump `rules_kotlin` version to v1.7.0-RC-3 to pick up changes from bazelbuild/rules_kotlin#810.
Risk Level: None
Testing: Existing tests using envoy_mobile_android_test macro.
Docs Changes: N/A
Release Notes: N/A

Signed-off-by: Rafal Augustyniak <[email protected]>
jpsim pushed a commit to envoyproxy/envoy that referenced this pull request Nov 29, 2022
Description: Add support for Kotlin robolectric tests by making `envoy_mobile_android_test` macro use `kt_android_local_test` maco as opposed to `android_local_test` macro as the former supports both Java and Kotlin files while the latter supports Java files only. The change should be additive when it comes to functionality. Bump `rules_kotlin` version to v1.7.0-RC-3 to pick up changes from bazelbuild/rules_kotlin#810.
Risk Level: None
Testing: Existing tests using envoy_mobile_android_test macro.
Docs Changes: N/A
Release Notes: N/A

Signed-off-by: Rafal Augustyniak <[email protected]>
Signed-off-by: JP Simard <[email protected]>
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.

2 participants