From b8a6a943212b557b4faa864dc16118a62424e91f Mon Sep 17 00:00:00 2001 From: Florian Weikert Date: Fri, 25 Sep 2015 12:36:08 +0000 Subject: Improved error reporting in RuleContext: - Unified duplicate code from RuleContext and RuleContext.Builder in a new class, RuleContext.ErrorReporter - Added the BUILD file location to error/warning messages if the offending rule was created by a macro -- MOS_MIGRATED_REVID=103934375 --- .../build/lib/analysis/mock/BazelAnalysisMock.java | 1 + .../build/lib/skylark/SkylarkRuleContextTest.java | 93 ++++++++++++++++++++++ .../devtools/build/lib/testutil/MoreAsserts.java | 4 +- 3 files changed, 97 insertions(+), 1 deletion(-) (limited to 'src/test/java') diff --git a/src/test/java/com/google/devtools/build/lib/analysis/mock/BazelAnalysisMock.java b/src/test/java/com/google/devtools/build/lib/analysis/mock/BazelAnalysisMock.java index b7b778a232..42e12bd8e0 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/mock/BazelAnalysisMock.java +++ b/src/test/java/com/google/devtools/build/lib/analysis/mock/BazelAnalysisMock.java @@ -71,6 +71,7 @@ public class BazelAnalysisMock extends AnalysisMock { "exports_files(['JavaBuilder_deploy.jar','SingleJar_deploy.jar',", " 'JavaBuilderCanary_deploy.jar', 'ijar', 'GenClass_deploy.jar'])"); config.create("tools/cpp/BUILD", + "cc_library(name = 'stl')", "filegroup(name = 'toolchain', ", " srcs = [':cc-compiler-local', ':cc-compiler-darwin', ':cc-compiler-piii',", " ':cc-compiler-armeabi-v7a', ':empty'],", diff --git a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleContextTest.java b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleContextTest.java index 9259ad7f45..b12dd9f88b 100644 --- a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleContextTest.java +++ b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleContextTest.java @@ -72,6 +72,99 @@ public class SkylarkRuleContextTest extends SkylarkTestCase { ")"); } + private void setUpAttributeErrorTest() throws Exception { + scratch.file("test/BUILD", + "load('/test/macros', 'macro_native_rule', 'macro_skylark_rule', 'skylark_rule')", + "macro_native_rule(name = 'm_native',", + " deps = [':jlib'])", + "macro_skylark_rule(name = 'm_skylark',", + " deps = [':jlib'])", + "java_library(name = 'jlib',", + " srcs = ['bla.java'])", + "cc_library(name = 'cclib',", + " deps = [':jlib'])", + "skylark_rule(name = 'skyrule',", + " deps = [':jlib'])"); + scratch.file("test/macros.bzl", + "def _impl(ctx):", + " return", + "skylark_rule = rule(", + " implementation = _impl,", + " attrs = {", + " 'deps': attr.label_list(providers = ['some_provider'], allow_files=True)", + " }", + ")", + "def macro_native_rule(name, deps): ", + " native.cc_library(name = name, deps = deps)", + "def macro_skylark_rule(name, deps):", + " skylark_rule(name = name, deps = deps)"); + reporter.removeHandler(failFastHandler); + } + + public void testNativeRuleAttributeErrorWithMacro() throws Exception { + setUpAttributeErrorTest(); + try { + createRuleContext("//test:m_native"); + fail("Should have failed because of invalid dependency"); + } catch (Exception ex) { + // Macro creates native rule -> location points to the rule and the message contains details + // about the macro. + assertContainsEvent( + "ERROR /workspace/test/BUILD:2:1: in deps attribute of cc_library rule //test:m_native: " + + "java_library rule '//test:jlib' is misplaced here (expected "); + // Skip the part of the error message that has details about the allowed deps since the mocks + // for the mac tests might have different values for them. + assertContainsEvent(". Since this " + + "rule was created by the macro 'macro_native_rule', the error might have been caused " + + "by the macro implementation in /workspace/test/macros.bzl:10:41"); + } + } + public void testSkylarkRuleAttributeErrorWithMacro() throws Exception { + setUpAttributeErrorTest(); + try { + createRuleContext("//test:m_skylark"); + fail("Should have failed because of invalid attribute value"); + } catch (Exception ex) { + // Macro creates Skylark rule -> location points to the rule and the message contains details + // about the macro. + assertContainsEvent( + "ERROR /workspace/test/BUILD:4:1: in deps attribute of skylark_rule rule " + + "//test:m_skylark: '//test:jlib' does not have mandatory provider 'some_provider'. " + + "Since this rule was created by the macro 'macro_skylark_rule', the error might have " + + "been caused by the macro implementation in /workspace/test/macros.bzl:12:36"); + } + } + public void testNativeRuleAttributeErrorWithoutMacro() throws Exception { + setUpAttributeErrorTest(); + try { + createRuleContext("//test:cclib"); + fail("Should have failed because of invalid dependency"); + } catch (Exception ex) { + // Native rule WITHOUT macro -> location points to the attribute and there is no mention of + // 'macro' at all. + assertContainsEvent("ERROR /workspace/test/BUILD:9:10: in deps attribute of " + + "cc_library rule //test:cclib: java_library rule '//test:jlib' is misplaced here " + + "(expected "); + // Skip the part of the error message that has details about the allowed deps since the mocks + // for the mac tests might have different values for them. + assertDoesNotContainEvent("Since this rule was created by the macro"); + } + } + + public void testSkylarkRuleAttributeErrorWithoutMacro() throws Exception { + setUpAttributeErrorTest(); + try { + createRuleContext("//test:skyrule"); + fail("Should have failed because of invalid dependency"); + } catch (Exception ex) { + // Skylark rule WITHOUT macro -> location points to the attribute and there is no mention of + // 'macro' at all. + assertContainsEvent("ERROR /workspace/test/BUILD:11:10: in deps attribute of " + + "skylark_rule rule //test:skyrule: '//test:jlib' does not have mandatory provider " + + "'some_provider'"); + } + } + public void testGetPrerequisiteArtifacts() throws Exception { SkylarkRuleContext ruleContext = createRuleContext("//foo:foo"); Object result = evalRuleContextCode(ruleContext, "ruleContext.files.srcs"); diff --git a/src/test/java/com/google/devtools/build/lib/testutil/MoreAsserts.java b/src/test/java/com/google/devtools/build/lib/testutil/MoreAsserts.java index c2c01d268d..a864694231 100644 --- a/src/test/java/com/google/devtools/build/lib/testutil/MoreAsserts.java +++ b/src/test/java/com/google/devtools/build/lib/testutil/MoreAsserts.java @@ -359,7 +359,9 @@ public class MoreAsserts { String expectedEvent, Set kinds) { for (Event event : eventCollector) { - if (event.getMessage().contains(expectedEvent) && kinds.contains(event.getKind())) { + // We want to be able to check for the location and the message type (error / warning). + // Consequently, we use toString() instead of getMessage(). + if (event.toString().contains(expectedEvent) && kinds.contains(event.getKind())) { return event; } } -- cgit v1.2.3