From 3c42b08e8440cc6899891ec4d650126988138973 Mon Sep 17 00:00:00 2001 From: Ben Hamilton Date: Fri, 15 Jan 2016 11:09:42 -0800 Subject: [PATCH] Improve error handling and include stderr in XctoolRunTestsStep Summary: I just upgraded Xcode on my local dev server. When I did, `buck test` started failing because the SDKs weren't installed correctly. It happened that `xctool` was the first thing to fail in my case, but we didn't include stderr when it failed. This updates `XctoolRunTestsStep` to include stderr when it fails. Test Plan: New tests added. `ant java-test -Dtest.class=XctoolRunTestsStepTest` Reviewed By: ryu2 fb-gh-sync-id: a6228f3 --- .../buck/apple/XctoolRunTestsStep.java | 92 ++++++++++--- .../buck/apple/XctoolRunTestsStepTest.java | 127 +++++++++++++++++- 2 files changed, 194 insertions(+), 25 deletions(-) diff --git a/src/com/facebook/buck/apple/XctoolRunTestsStep.java b/src/com/facebook/buck/apple/XctoolRunTestsStep.java index 34cb695e1f3..c6339dae664 100644 --- a/src/com/facebook/buck/apple/XctoolRunTestsStep.java +++ b/src/com/facebook/buck/apple/XctoolRunTestsStep.java @@ -23,6 +23,7 @@ import com.facebook.buck.step.Step; import com.facebook.buck.test.selectors.TestDescription; import com.facebook.buck.test.selectors.TestSelectorList; +import com.facebook.buck.util.Console; import com.facebook.buck.util.Escaper; import com.facebook.buck.util.MoreThrowables; import com.facebook.buck.util.ProcessExecutor; @@ -37,6 +38,7 @@ import com.google.common.collect.Multimap; import com.google.common.collect.Iterables; import com.google.common.io.ByteStreams; +import com.google.common.io.CharStreams; import java.io.BufferedReader; import java.io.IOException; @@ -47,7 +49,7 @@ import java.nio.file.Path; import java.util.Collection; import java.util.HashMap; -import java.util.List; +import java.util.Locale; import java.util.Map; import java.util.concurrent.Executors; import java.util.concurrent.ScheduledExecutorService; @@ -244,17 +246,26 @@ public int execute(ExecutionContext context) throws InterruptedException { if (!testSelectorList.isEmpty()) { try { - List xctoolFilterParams = listAndFilterTestsThenFormatXctoolParams( + ImmutableList.Builder xctoolFilterParamsBuilder = ImmutableList.builder(); + int returnCode = listAndFilterTestsThenFormatXctoolParams( context.getProcessExecutor(), + context.getConsole(), testSelectorList, // Copy the entire xctool command and environment but add a -listTestsOnly arg. ProcessExecutorParams.builder() .from(processExecutorParamsBuilder.build()) .addCommand("-listTestsOnly") - .build()); + .build(), + xctoolFilterParamsBuilder); + if (returnCode != 0) { + context.getConsole().printErrorText("Failed to query tests with xctool"); + return returnCode; + } + ImmutableList xctoolFilterParams = xctoolFilterParamsBuilder.build(); if (xctoolFilterParams.isEmpty()) { context.getConsole().printBuildFailure( String.format( + Locale.US, "No tests found matching specified filter (%s)", testSelectorList.getExplanation())); return 0; @@ -282,10 +293,15 @@ public int execute(ExecutionContext context) throws InterruptedException { context.getProcessExecutor().launchProcess(processExecutorParams); int exitCode; + String stderr; try (OutputStream outputStream = filesystem.newFileOutputStream( outputPath); TeeInputStream stdoutWrapperStream = new TeeInputStream( - launchedProcess.getInputStream(), outputStream)) { + launchedProcess.getInputStream(), outputStream); + InputStreamReader stderrReader = new InputStreamReader( + launchedProcess.getErrorStream(), + StandardCharsets.UTF_8); + BufferedReader bufferedStderrReader = new BufferedReader(stderrReader)) { if (stdoutReadingCallback.isPresent()) { // The caller is responsible for reading all the data, which TeeInputStream will // copy to outputStream. @@ -296,15 +312,29 @@ public int execute(ExecutionContext context) throws InterruptedException { stdoutWrapperStream.close(); ByteStreams.copy(launchedProcess.getInputStream(), outputStream); } + stderr = CharStreams.toString(bufferedStderrReader).trim(); exitCode = waitForProcessAndGetExitCode(context.getProcessExecutor(), launchedProcess); - LOG.debug("Finished running command, exit code %d", exitCode); + LOG.debug("Finished running command, exit code %d, stderr %s", exitCode, stderr); } finally { context.getProcessExecutor().destroyLaunchedProcess(launchedProcess); context.getProcessExecutor().waitForLaunchedProcess(launchedProcess); } if (exitCode != 0) { - LOG.warn("%s exited with error %d", command, exitCode); + if (!stderr.isEmpty()) { + context.getConsole().printErrorText( + String.format( + Locale.US, + "xctool failed with exit code %d: %s", + exitCode, + stderr)); + } else { + context.getConsole().printErrorText( + String.format( + Locale.US, + "xctool failed with exit code %d", + exitCode)); + } } return exitCode; @@ -325,10 +355,12 @@ public String getDescription(ExecutionContext context) { return Joiner.on(' ').join(Iterables.transform(command, Escaper.SHELL_ESCAPER)); } - private static List listAndFilterTestsThenFormatXctoolParams( + private static int listAndFilterTestsThenFormatXctoolParams( ProcessExecutor processExecutor, + Console console, TestSelectorList testSelectorList, - ProcessExecutorParams listTestsOnlyParams) throws IOException, InterruptedException { + ProcessExecutorParams listTestsOnlyParams, + ImmutableList.Builder filterParamsBuilder) throws IOException, InterruptedException { Preconditions.checkArgument(!testSelectorList.isEmpty()); LOG.debug("Filtering tests with selector list: %s", testSelectorList.getExplanation()); @@ -337,31 +369,50 @@ private static List listAndFilterTestsThenFormatXctoolParams( processExecutor.launchProcess(listTestsOnlyParams); ListTestsOnlyHandler listTestsOnlyHandler = new ListTestsOnlyHandler(); + String stderr; + int listTestsResult; try (InputStreamReader isr = new InputStreamReader( launchedProcess.getInputStream(), StandardCharsets.UTF_8); - BufferedReader br = new BufferedReader(isr)) { + BufferedReader br = new BufferedReader(isr); + InputStreamReader esr = + new InputStreamReader( + launchedProcess.getErrorStream(), + StandardCharsets.UTF_8); + BufferedReader ebr = new BufferedReader(esr)) { XctoolOutputParsing.streamOutputFromReader(br, listTestsOnlyHandler); + stderr = CharStreams.toString(ebr).trim(); + listTestsResult = processExecutor.waitForLaunchedProcess(launchedProcess); } - int listTestsResult = processExecutor.waitForLaunchedProcess(launchedProcess); if (listTestsResult != 0) { - throw new IOException( - String.format( - "Process %s exited with error %d", - listTestsOnlyParams.getCommand(), - listTestsResult)); + if (!stderr.isEmpty()) { + console.printErrorText( + String.format( + Locale.US, + "xctool failed with exit code %d: %s", + listTestsResult, + stderr)); + } else { + console.printErrorText( + String.format( + Locale.US, + "xctool failed with exit code %d", + listTestsResult)); + } + } else { + formatXctoolFilterParams( + testSelectorList, listTestsOnlyHandler.testTargetsToDescriptions, filterParamsBuilder); } - return formatXctoolFilterParams( - testSelectorList, listTestsOnlyHandler.testTargetsToDescriptions); + return listTestsResult; } - private static List formatXctoolFilterParams( + private static void formatXctoolFilterParams( TestSelectorList testSelectorList, - Multimap testTargetsToDescriptions) { - ImmutableList.Builder filterParamsBuilder = ImmutableList.builder(); + Multimap testTargetsToDescriptions, + ImmutableList.Builder filterParamsBuilder) { for (String testTarget : testTargetsToDescriptions.keySet()) { StringBuilder sb = new StringBuilder(); boolean matched = false; @@ -385,7 +436,6 @@ private static List formatXctoolFilterParams( filterParamsBuilder.add(sb.toString()); } } - return filterParamsBuilder.build(); } private static ImmutableList createCommandArgs( diff --git a/test/com/facebook/buck/apple/XctoolRunTestsStepTest.java b/test/com/facebook/buck/apple/XctoolRunTestsStepTest.java index 63ef29c12fa..104278a5af1 100644 --- a/test/com/facebook/buck/apple/XctoolRunTestsStepTest.java +++ b/test/com/facebook/buck/apple/XctoolRunTestsStepTest.java @@ -16,6 +16,8 @@ package com.facebook.buck.apple; +import static org.hamcrest.Matchers.allOf; +import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.not; import static org.hamcrest.Matchers.nullValue; @@ -25,6 +27,7 @@ import com.facebook.buck.step.TestExecutionContext; import com.facebook.buck.test.selectors.TestSelectorList; import com.facebook.buck.testutil.FakeProjectFilesystem; +import com.facebook.buck.testutil.TestConsole; import com.facebook.buck.util.FakeProcess; import com.facebook.buck.util.FakeProcessExecutor; import com.facebook.buck.util.ProcessExecutorParams; @@ -91,6 +94,58 @@ public void xctoolCommandWithOnlyLogicTests() throws Exception { equalTo(0)); } + @Test + public void xctoolCommandWhichFailsPrintsStderrToConsole() throws Exception { + FakeProjectFilesystem projectFilesystem = new FakeProjectFilesystem(); + XctoolRunTestsStep step = new XctoolRunTestsStep( + projectFilesystem, + Paths.get("/path/to/xctool"), + Optional.absent(), + "iphonesimulator", + Optional.absent(), + ImmutableSet.of(Paths.get("/path/to/Foo.xctest")), + ImmutableMap.of(), + Paths.get("/path/to/output.json"), + Optional.absent(), + Suppliers.ofInstance(Optional.of(Paths.get("/path/to/developer/dir"))), + TestSelectorList.EMPTY, + Optional.absent(), + Optional.absent(), + Optional.absent(), + Optional.absent()); + ProcessExecutorParams xctoolParams = + ProcessExecutorParams.builder() + .setCommand( + ImmutableList.of( + "/path/to/xctool", + "-reporter", + "json-stream", + "-sdk", + "iphonesimulator", + "run-tests", + "-logicTest", + "/path/to/Foo.xctest")) + .setEnvironment(ImmutableMap.of("DEVELOPER_DIR", "/path/to/developer/dir")) + .setDirectory(projectFilesystem.getRootPath().toAbsolutePath().toFile()) + .setRedirectOutput(ProcessBuilder.Redirect.PIPE) + .build(); + FakeProcess fakeXctoolFailure = new FakeProcess(42, "", "Something went terribly wrong\n"); + FakeProcessExecutor processExecutor = new FakeProcessExecutor( + ImmutableMap.of(xctoolParams, fakeXctoolFailure)); + TestConsole testConsole = new TestConsole(); + ExecutionContext executionContext = TestExecutionContext.newBuilder() + .setProcessExecutor(processExecutor) + .setEnvironment(ImmutableMap.of()) + .setConsole(testConsole) + .build(); + assertThat( + step.execute(executionContext), + equalTo(42)); + assertThat( + testConsole.getTextWrittenToStdErr(), + equalTo("xctool failed with exit code 42: Something went terribly wrong\n")); + } + @Test public void xctoolCommandWithOnlyAppTests() throws Exception { FakeProjectFilesystem projectFilesystem = new FakeProjectFilesystem(); @@ -234,9 +289,11 @@ public void xctoolCommandWhichReturnsExitCode1DoesNotFailStep() throws Exception .setDirectory(projectFilesystem.getRootPath().toAbsolutePath().toFile()) .setRedirectOutput(ProcessBuilder.Redirect.PIPE) .build(); - FakeProcess fakeXctoolSuccess = new FakeProcess(1, "", ""); + // Test failure is indicated by xctool exiting with exit code 1, so it shouldn't + // fail the step. + FakeProcess fakeXctoolTestFailure = new FakeProcess(1, "", ""); FakeProcessExecutor processExecutor = new FakeProcessExecutor( - ImmutableMap.of(xctoolParams, fakeXctoolSuccess)); + ImmutableMap.of(xctoolParams, fakeXctoolTestFailure)); ExecutionContext executionContext = TestExecutionContext.newBuilder() .setProcessExecutor(processExecutor) .setEnvironment(ImmutableMap.of()) @@ -282,9 +339,9 @@ public void xctoolCommandWhichReturnsExitCode400FailsStep() throws Exception { .setDirectory(projectFilesystem.getRootPath().toAbsolutePath().toFile()) .setRedirectOutput(ProcessBuilder.Redirect.PIPE) .build(); - FakeProcess fakeXctoolSuccess = new FakeProcess(400, "", ""); + FakeProcess fakeXctoolFailure = new FakeProcess(400, "", ""); FakeProcessExecutor processExecutor = new FakeProcessExecutor( - ImmutableMap.of(xctoolParams, fakeXctoolSuccess)); + ImmutableMap.of(xctoolParams, fakeXctoolFailure)); ExecutionContext executionContext = TestExecutionContext.newBuilder() .setProcessExecutor(processExecutor) .setEnvironment(ImmutableMap.of()) @@ -383,6 +440,68 @@ public void xctoolCommandWithTestSelectorFiltersTests() throws Exception { } } + @Test + public void xctoolCommandWithTestSelectorFailsIfListTestsOnlyFails() throws Exception { + FakeProjectFilesystem projectFilesystem = new FakeProjectFilesystem(); + XctoolRunTestsStep step = new XctoolRunTestsStep( + projectFilesystem, + Paths.get("/path/to/xctool"), + Optional.absent(), + "iphonesimulator", + Optional.absent(), + ImmutableSet.of(Paths.get("/path/to/FooTest.xctest"), Paths.get("/path/to/BarTest.xctest")), + ImmutableMap.of(), + Paths.get("/path/to/output.json"), + Optional.absent(), + Suppliers.ofInstance(Optional.of(Paths.get("/path/to/developer/dir"))), + TestSelectorList.builder() + .addRawSelectors("#.*Magic.*") + .build(), + Optional.absent(), + Optional.absent(), + Optional.absent(), + Optional.absent()); + + ProcessExecutorParams xctoolListOnlyParams = + ProcessExecutorParams.builder() + .setCommand( + ImmutableList.of( + "/path/to/xctool", + "-reporter", + "json-stream", + "-sdk", + "iphonesimulator", + "run-tests", + "-logicTest", + "/path/to/FooTest.xctest", + "-logicTest", + "/path/to/BarTest.xctest", + "-listTestsOnly")) + .setEnvironment(ImmutableMap.of("DEVELOPER_DIR", "/path/to/developer/dir")) + .setDirectory(projectFilesystem.getRootPath().toAbsolutePath().toFile()) + .setRedirectOutput(ProcessBuilder.Redirect.PIPE) + .build(); + FakeProcess fakeXctoolListTestsFailureProcess = + new FakeProcess(42, "", "Chopper Dave, we have Uh-Oh"); + FakeProcessExecutor processExecutor = new FakeProcessExecutor( + ImmutableMap.of( + xctoolListOnlyParams, fakeXctoolListTestsFailureProcess)); + TestConsole testConsole = new TestConsole(); + ExecutionContext executionContext = TestExecutionContext.newBuilder() + .setProcessExecutor(processExecutor) + .setEnvironment(ImmutableMap.of()) + .setConsole(testConsole) + .build(); + assertThat( + step.execute(executionContext), + equalTo(42)); + assertThat( + testConsole.getTextWrittenToStdErr(), + allOf( + containsString("xctool failed with exit code 42: Chopper Dave, we have Uh-Oh"), + containsString("Failed to query tests with xctool"))); + } + @Test public void xctoolCommandWithTestSelectorMatchingNothingDoesNotFail() throws Exception { FakeProjectFilesystem projectFilesystem = new FakeProjectFilesystem();