diff --git a/src/main/java/com/google/devtools/build/lib/analysis/test/AnalysisTestActionBuilder.java b/src/main/java/com/google/devtools/build/lib/analysis/test/AnalysisTestActionBuilder.java index 18a9eb6556a4a2..7e0a0d7a9ed48d 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/test/AnalysisTestActionBuilder.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/test/AnalysisTestActionBuilder.java @@ -1,3 +1,4 @@ + // Copyright 2018 The Bazel Authors. All rights reserved. // // Licensed under the Apache License, Version 2.0 (the "License"); @@ -20,49 +21,50 @@ import com.google.devtools.build.lib.util.OS; /** - * Helper for writing test actions for analysis test rules. Analysis test rules are - * restricted to disallow the rule implementation functions from registering actions themselves; - * such rules register test success/failure via {@link AnalysisTestResultInfo}. This helper - * registers the appropriate test script simulating success or failure of the test. + * Helper for writing test actions for analysis test rules. Analysis test rules are restricted to + * disallow the rule implementation functions from registering actions themselves; such rules + * register test success/failure via {@link AnalysisTestResultInfo}. This helper registers the + * appropriate test script simulating success or failure of the test. */ public class AnalysisTestActionBuilder { + private AnalysisTestActionBuilder() {} + /** - * Register and return an action to write a test script to the default executable location - * reflecting the given info object. + * Register an action to write a test script to the default executable location. The script should + * return exit status 0 if the test passed. It should print the failure message and return exit + * status 1 if the test failed. */ - public static FileWriteAction writeAnalysisTestAction( - RuleContext ruleContext, - AnalysisTestResultInfo infoObject) { - FileWriteAction action; + public static void writeAnalysisTestAction( + RuleContext ruleContext, AnalysisTestResultInfo testResultInfo) { // TODO(laszlocsomor): Use the execution platform, not the host platform. boolean isExecutedOnWindows = OS.getCurrent() == OS.WINDOWS; - + String escapedMessage = + isExecutedOnWindows + ? testResultInfo.getMessage().replace("%", "%%") + // Prefix each character with \ (double-escaped; once in the string, once in the + // replacement sequence, which allows backslash-escaping literal "$"). "." is put in + // parentheses because ErrorProne is overly vigorous about objecting to "." as an + // always-matching regex (b/201772278). + : testResultInfo.getMessage().replaceAll("(.)", "\\\\$1"); + StringBuilder sb = new StringBuilder(); if (isExecutedOnWindows) { - StringBuilder sb = new StringBuilder().append("@echo off\n"); - for (String line : Splitter.on("\n").split(infoObject.getMessage())) { - sb.append("echo ").append(line).append("\n"); - } - String content = sb - .append("exit /b ").append(infoObject.getSuccess() ? "0" : "1") - .toString(); - - action = FileWriteAction.create(ruleContext, - ruleContext.createOutputArtifactScript(), content, /* executable */ true); - - } else { - String content = - "cat << EOF\n" - + infoObject.getMessage() - + "\n" - + "EOF\n" - + "exit " - + (infoObject.getSuccess() ? "0" : "1"); - action = FileWriteAction.create(ruleContext, - ruleContext.createOutputArtifactScript(), content, /* executable */ true); + sb.append("@echo off\n"); } - + for (String line : Splitter.on("\n").split(escapedMessage)) { + sb.append("echo ").append(line).append("\n"); + } + sb.append("exit "); + if (isExecutedOnWindows) { + sb.append("/b "); + } + sb.append(testResultInfo.getSuccess() ? "0" : "1"); + FileWriteAction action = + FileWriteAction.create( + ruleContext, + ruleContext.createOutputArtifactScript(), + sb.toString(), + /*makeExecutable=*/ true); ruleContext.registerAction(action); - return action; } } diff --git a/src/test/shell/integration/analysis_test_test.sh b/src/test/shell/integration/analysis_test_test.sh index 1ad2f638b2184f..032d9bd1a6236f 100755 --- a/src/test/shell/integration/analysis_test_test.sh +++ b/src/test/shell/integration/analysis_test_test.sh @@ -113,6 +113,96 @@ EOF expect_log "A failure message!" } +function test_failing_test_shell_escape_in_message() { + mkdir -p package + cat > package/test.bzl <<'EOF' +def _rule_test_impl(ctx): + return [AnalysisTestResultInfo(success = False, + message = 'Command is not "$1 copy $2"')] + +my_rule_test = rule( + implementation = _rule_test_impl, + analysis_test = True, +) +EOF + + cat > package/BUILD <& "$TEST_log" || fail "Unexpected success" + + expect_log "FAILED" + + cat "${PRODUCT_NAME}-testlogs/package/r/test.log" > "$TEST_log" + + expect_log 'Command is not "$1 copy $2"' +} + +function test_failing_test_cmd_escape_in_message() { + mkdir -p package + cat > package/test.bzl <<'EOF' +def _rule_test_impl(ctx): + return [ + AnalysisTestResultInfo( + success = False, + message = 'Command should contain "\\ & < > | ^ ! %FOO%"', + ), + ] + +my_rule_test = rule( + implementation = _rule_test_impl, + analysis_test = True, +) +EOF + + cat > package/BUILD <& "$TEST_log" || fail "Unexpected success" + + expect_log "FAILED" + + cat "${PRODUCT_NAME}-testlogs/package/r/test.log" > "$TEST_log" + + expect_log 'Command should contain "\\ & < > \| ^ ! %FOO%"' +} + +function test_failing_test_eof_string_in_message() { + mkdir -p package + cat > package/test.bzl <<'EOF' +def _rule_test_impl(ctx): + return [AnalysisTestResultInfo(success = False, + message = '"\nEOF\n" not in command')] + +my_rule_test = rule( + implementation = _rule_test_impl, + analysis_test = True, +) +EOF + + cat > package/BUILD <& "$TEST_log" || fail "Unexpected success" + + expect_log "FAILED" + + cat "${PRODUCT_NAME}-testlogs/package/r/test.log" > "$TEST_log" + + # expect_log uses grep and looks at individual lines, but we can make sure + # the part after \nEOF\n isn't cut off, as it was previously. + expect_log "\" not in command" +} + function test_expected_failure_test() { mkdir -p package cat > package/test.bzl <