From 60ddaaaa7ecea30a3e72a989abc5f33afc076b10 Mon Sep 17 00:00:00 2001 From: David Date: Wed, 21 Feb 2024 11:52:59 -0700 Subject: [PATCH 1/3] fix `testonly` with js_binary the current logic is wrong because it tries to test `js_binary` when `testonly` is passed. We actually want the inverse which is we don't want to run js_binary as a test if it is marked as test_only --- js/private/js_binary.bzl | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/js/private/js_binary.bzl b/js/private/js_binary.bzl index 1e0a55382..785da4b75 100644 --- a/js/private/js_binary.bzl +++ b/js/private/js_binary.bzl @@ -559,11 +559,8 @@ def _js_binary_impl(ctx): providers = [] - if ctx.attr.testonly and ctx.configuration.coverage_enabled: - # We have to instruct rule implementers to have this attribute present. - if not hasattr(ctx.attr, "_lcov_merger"): - fail("_lcov_merger attribute is missing and coverage was requested") - + # We have to instruct test rule implementers to have this attribute present. + if ctx.attr.testonly and ctx.configuration.coverage_enabled and hasattr(ctx.attr, "_lcov_merger"): # We have to propagate _lcov_merger runfiles since bazel does not treat _lcov_merger as a proper tool. # See: https://github.com/bazelbuild/bazel/issues/4033 runfiles = runfiles.merge(ctx.attr._lcov_merger[DefaultInfo].default_runfiles) From 4c1cb9bfb4a3987107c4f4f6afda483b2f2d6b5e Mon Sep 17 00:00:00 2001 From: David Date: Wed, 21 Feb 2024 15:14:38 -0700 Subject: [PATCH 2/3] refactor to use _is_test attribute instead This addresses the problem in a different way that allows us to not change the existing logic, but instead differentiate one step further with attributes --- js/private/js_binary.bzl | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/js/private/js_binary.bzl b/js/private/js_binary.bzl index 785da4b75..c2cd09fdc 100644 --- a/js/private/js_binary.bzl +++ b/js/private/js_binary.bzl @@ -559,8 +559,10 @@ def _js_binary_impl(ctx): providers = [] - # We have to instruct test rule implementers to have this attribute present. - if ctx.attr.testonly and ctx.configuration.coverage_enabled and hasattr(ctx.attr, "_lcov_merger"): + if ctx.attr.testonly and ctx.configuration.coverage_enabled and hasattr(ctx.attr, "_is_test"): + # We have to instruct rule implementers to have this attribute present. + if not hasattr(ctx.attr, "_lcov_merger"): + fail("_lcov_merger attribute is missing and coverage was requested") # We have to propagate _lcov_merger runfiles since bazel does not treat _lcov_merger as a proper tool. # See: https://github.com/bazelbuild/bazel/issues/4033 runfiles = runfiles.merge(ctx.attr._lcov_merger[DefaultInfo].default_runfiles) @@ -638,6 +640,7 @@ the contract between Bazel and a test runner.""", default = Label("//js/private/coverage:merger"), cfg = "exec", ), + "_is_test": True }), test = True, toolchains = js_binary_lib.toolchains, From 7c5a071c281d562a22784f2788964b4a0610c793 Mon Sep 17 00:00:00 2001 From: David Date: Wed, 21 Feb 2024 15:20:39 -0700 Subject: [PATCH 3/3] Update js/private/js_binary.bzl --- js/private/js_binary.bzl | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/js/private/js_binary.bzl b/js/private/js_binary.bzl index c2cd09fdc..0456212e3 100644 --- a/js/private/js_binary.bzl +++ b/js/private/js_binary.bzl @@ -640,7 +640,9 @@ the contract between Bazel and a test runner.""", default = Label("//js/private/coverage:merger"), cfg = "exec", ), - "_is_test": True + "_is_test": attr.bool( + default = True + ) }), test = True, toolchains = js_binary_lib.toolchains,