From 2e5498d23462bb6a473fce661ba0ddfecd53cd89 Mon Sep 17 00:00:00 2001 From: Andrew Parmet Date: Thu, 12 May 2022 15:27:27 -0400 Subject: [PATCH] improve error messages (#73) --- .../kotlin/com/parmet/buf/gradle/Breaking.kt | 7 ++++- .../com/parmet/buf/gradle/BufSupport.kt | 25 ++++++++++++++-- .../DirectorySpecificBufExecutionSupport.kt | 21 ++++++++------ .../kotlin/com/parmet/buf/gradle/Format.kt | 8 ++++- src/main/kotlin/com/parmet/buf/gradle/Lint.kt | 7 ++++- .../parmet/buf/gradle/AbstractBreakingTest.kt | 6 ++++ .../buf/gradle/AbstractFormatCheckTest.kt | 29 +++++++++---------- .../com/parmet/buf/gradle/AbstractLintTest.kt | 6 ++++ .../FormatCheckWithProtobufGradleTest.kt | 21 ++------------ .../main/proto/parmet/buf/test/v1/test.proto | 6 ++-- .../subdir/parmet/buf/test/v1/test.proto | 3 +- 11 files changed, 86 insertions(+), 53 deletions(-) diff --git a/src/main/kotlin/com/parmet/buf/gradle/Breaking.kt b/src/main/kotlin/com/parmet/buf/gradle/Breaking.kt index 72a2a5d9..3bd78d16 100644 --- a/src/main/kotlin/com/parmet/buf/gradle/Breaking.kt +++ b/src/main/kotlin/com/parmet/buf/gradle/Breaking.kt @@ -62,6 +62,11 @@ private fun Project.configureBreakingTask() { bufBuildPublicationFile, "--against", singleFileFromConfiguration(BUF_BREAKING_CONFIGURATION_NAME) - ) + ) { + """ + |Some Protobuf files had breaking changes: + |$it + """.trimMargin() + } } } diff --git a/src/main/kotlin/com/parmet/buf/gradle/BufSupport.kt b/src/main/kotlin/com/parmet/buf/gradle/BufSupport.kt index 67e02e8e..51e77bb5 100644 --- a/src/main/kotlin/com/parmet/buf/gradle/BufSupport.kt +++ b/src/main/kotlin/com/parmet/buf/gradle/BufSupport.kt @@ -19,6 +19,8 @@ import org.gradle.api.Project import org.gradle.api.Task import org.gradle.kotlin.dsl.ivy import org.gradle.kotlin.dsl.repositories +import java.io.ByteArrayOutputStream +import java.nio.charset.StandardCharsets const val BUF_BINARY_CONFIGURATION_NAME = "bufTool" @@ -54,17 +56,19 @@ internal fun Project.configureBufDependency() { ) } -internal fun Task.execBuf(vararg args: Any) { - execBuf(args.asList()) +internal fun Task.execBuf(vararg args: Any, customErrorMessage: ((String) -> String)? = null) { + execBuf(args.asList(), customErrorMessage) } -internal fun Task.execBuf(args: Iterable) { +internal fun Task.execBuf(args: Iterable, customErrorMessage: ((String) -> String)? = null) { if (project.hasProtobufGradlePlugin()) { dependsOn(CREATE_SYM_LINKS_TO_MODULES_TASK_NAME) dependsOn(WRITE_WORKSPACE_YAML_TASK_NAME) } doLast { with(project) { + val out = ByteArrayOutputStream() + exec { val executable = singleFileFromConfiguration(BUF_BINARY_CONFIGURATION_NAME) @@ -82,8 +86,23 @@ internal fun Task.execBuf(args: Iterable) { commandLine(executable) setArgs(args) + isIgnoreExitValue = true + + if (customErrorMessage != null) { + standardOutput = out + } logger.info("Running buf from $workingDir: `buf ${args.joinToString(" ")}`") + }.also { + if (customErrorMessage == null) { + it.assertNormalExitValue() + } else { + if (it.exitValue != 0) { + throw IllegalStateException( + customErrorMessage(out.toByteArray().toString(StandardCharsets.UTF_8)) + ) + } + } } } } diff --git a/src/main/kotlin/com/parmet/buf/gradle/DirectorySpecificBufExecutionSupport.kt b/src/main/kotlin/com/parmet/buf/gradle/DirectorySpecificBufExecutionSupport.kt index ba2dbe8d..aa0b02eb 100644 --- a/src/main/kotlin/com/parmet/buf/gradle/DirectorySpecificBufExecutionSupport.kt +++ b/src/main/kotlin/com/parmet/buf/gradle/DirectorySpecificBufExecutionSupport.kt @@ -19,31 +19,34 @@ import org.gradle.api.Task import java.nio.file.Path internal fun Task.configureDirectorySpecificBufExecution( - vararg bufCommand: String + vararg bufCommand: String, + customErrorMessage: ((String) -> String)? = null ) { - configureDirectorySpecificBufExecution(bufCommand.asList(), emptyList()) + configureDirectorySpecificBufExecution(bufCommand.asList(), emptyList(), customErrorMessage) } internal fun Task.configureDirectorySpecificBufExecution( bufCommand: String, - extraArgs: Iterable + extraArgs: Iterable, + customErrorMessage: ((String) -> String) ) { - configureDirectorySpecificBufExecution(listOf(bufCommand), extraArgs) + configureDirectorySpecificBufExecution(listOf(bufCommand), extraArgs, customErrorMessage) } private fun Task.configureDirectorySpecificBufExecution( bufCommand: Iterable, - extraArgs: Iterable + extraArgs: Iterable, + customErrorMessage: ((String) -> String)? = null ) { - fun lintWithArgs(path: Path? = null) = + fun runWithArgs(path: Path? = null) = bufCommand + listOfNotNull(path?.let(::mangle)) + extraArgs when { project.hasProtobufGradlePlugin() -> - project.srcProtoDirs().forEach { execBuf(lintWithArgs(it)) } + project.srcProtoDirs().forEach { execBuf(runWithArgs(it), customErrorMessage) } project.hasWorkspace() -> - execBuf(bufCommand) + execBuf(bufCommand, customErrorMessage) else -> - execBuf(lintWithArgs()) + execBuf(runWithArgs(), customErrorMessage) } } diff --git a/src/main/kotlin/com/parmet/buf/gradle/Format.kt b/src/main/kotlin/com/parmet/buf/gradle/Format.kt index aec94e4b..d05df29c 100644 --- a/src/main/kotlin/com/parmet/buf/gradle/Format.kt +++ b/src/main/kotlin/com/parmet/buf/gradle/Format.kt @@ -33,7 +33,13 @@ private fun Project.configureBufFormatCheck() { description = "Checks that a Protobuf schema is formatted according to Buf's formatting rules." enabled = getExtension().enforceFormat - configureDirectorySpecificBufExecution("format", "-d", "--exit-code") + configureDirectorySpecificBufExecution("format", "-d", "--exit-code") { + """ + |Some Protobuf files had format violations: + |$it + |Run './gradlew :bufFormatApply' to fix these violations. + """.trimMargin() + } } tasks.named(CHECK_TASK_NAME).dependsOn(BUF_FORMAT_CHECK_TASK_NAME) diff --git a/src/main/kotlin/com/parmet/buf/gradle/Lint.kt b/src/main/kotlin/com/parmet/buf/gradle/Lint.kt index cc670fa3..b02ca541 100644 --- a/src/main/kotlin/com/parmet/buf/gradle/Lint.kt +++ b/src/main/kotlin/com/parmet/buf/gradle/Lint.kt @@ -29,7 +29,12 @@ internal fun Project.configureLint() { configureDirectorySpecificBufExecution( "lint", bufConfigFile()?.let { listOf("--config", it.readText()) }.orEmpty() - ) + ) { + """ + |Some Protobuf files had lint violations: + |$it + """.trimMargin() + } } tasks.named(CHECK_TASK_NAME).dependsOn(BUF_LINT_TASK_NAME) diff --git a/src/test/kotlin/com/parmet/buf/gradle/AbstractBreakingTest.kt b/src/test/kotlin/com/parmet/buf/gradle/AbstractBreakingTest.kt index e41fafa3..6cadf552 100644 --- a/src/test/kotlin/com/parmet/buf/gradle/AbstractBreakingTest.kt +++ b/src/test/kotlin/com/parmet/buf/gradle/AbstractBreakingTest.kt @@ -58,6 +58,12 @@ abstract class AbstractBreakingTest : AbstractBufIntegrationTest() { val result = checkRunner().buildAndFail() assertThat(result.task(":$BUF_BREAKING_TASK_NAME")?.outcome).isEqualTo(FAILED) + assertThat(result.output).contains( + """ + Execution failed for task ':bufBreaking'. + > Some Protobuf files had breaking changes: + """.trimIndent() + ) assertThat(result.output).contains("Previously present message \"BasicMessage\" was deleted from file.") } diff --git a/src/test/kotlin/com/parmet/buf/gradle/AbstractFormatCheckTest.kt b/src/test/kotlin/com/parmet/buf/gradle/AbstractFormatCheckTest.kt index 2a611098..afde719e 100644 --- a/src/test/kotlin/com/parmet/buf/gradle/AbstractFormatCheckTest.kt +++ b/src/test/kotlin/com/parmet/buf/gradle/AbstractFormatCheckTest.kt @@ -29,33 +29,32 @@ abstract class AbstractFormatCheckTest : AbstractBufIntegrationTest() { @Test fun `format an incorrect message`() { - baseAssertBadWhitespace() + assertBadWhitespace() } @Test fun `do not format an incorrect message when enforcement is disabled`() { - baseAssertBadWhitespace() + assertBadWhitespace() buildFile.replace("enforceFormat = true", "enforceFormat = false") assertSuccess() } - private fun baseAssertBadWhitespace() { - assertBadWhitespace( - """ - -message Foo { - - - -} - +message Foo {} - """.trimIndent() - ) - } - - protected fun assertBadWhitespace(diff: String) { + protected fun assertBadWhitespace() { val result = checkRunner().buildAndFail() assertThat(result.task(":$BUF_FORMAT_CHECK_TASK_NAME")?.outcome).isEqualTo(FAILED) - assertThat(result.output).contains(diff) + assertThat(result.output) + .contains( + """ + | -message Foo { + | - + | -} + | +message Foo {} + | + | Run './gradlew :bufFormatApply' to fix these violations. + """.trimMargin() + ) } protected fun assertSuccess() { diff --git a/src/test/kotlin/com/parmet/buf/gradle/AbstractLintTest.kt b/src/test/kotlin/com/parmet/buf/gradle/AbstractLintTest.kt index f6881a84..4d5d126c 100644 --- a/src/test/kotlin/com/parmet/buf/gradle/AbstractLintTest.kt +++ b/src/test/kotlin/com/parmet/buf/gradle/AbstractLintTest.kt @@ -40,6 +40,12 @@ abstract class AbstractLintTest : LintTestUtilities, AbstractBufIntegrationTest( private fun assertBadEnumSuffix() { val result = checkRunner().buildAndFail() assertThat(result.task(":$BUF_LINT_TASK_NAME")?.outcome).isEqualTo(FAILED) + assertThat(result.output).contains( + """ + Execution failed for task ':bufLint'. + > Some Protobuf files had lint violations: + """.trimIndent() + ) assertThat(result.output).contains("Enum zero value name \"TEST_FOO\" should be suffixed with \"_UNSPECIFIED\"") } } diff --git a/src/test/kotlin/com/parmet/buf/gradle/FormatCheckWithProtobufGradleTest.kt b/src/test/kotlin/com/parmet/buf/gradle/FormatCheckWithProtobufGradleTest.kt index afe47d4e..f9dae39f 100644 --- a/src/test/kotlin/com/parmet/buf/gradle/FormatCheckWithProtobufGradleTest.kt +++ b/src/test/kotlin/com/parmet/buf/gradle/FormatCheckWithProtobufGradleTest.kt @@ -35,15 +35,7 @@ class FormatCheckWithProtobufGradleTest : AbstractFormatCheckTest() { @Test fun `format a bad separate protobuf source directory through the protobuf-gradle-plugin`() { - assertBadWhitespace( - """ - -message BasicMessage { - - - - - -} - +message BasicMessage {} - """.trimIndent() - ) + assertBadWhitespace() } @Test @@ -53,15 +45,6 @@ class FormatCheckWithProtobufGradleTest : AbstractFormatCheckTest() { @Test fun `format a bad file with a protobuf dependency and a google dependency with the protobuf-gradle-plugin`() { - assertBadWhitespace( - """ - message BasicMessage { - - - - - google.protobuf.Any any = 1; - protokt.ProtoktFileOptions protokt_file_options = 2; - } - """.trimIndent() - ) + assertBadWhitespace() } } diff --git a/src/test/resources/FormatCheckWithProtobufGradleTest/format a bad file with a protobuf dependency and a google dependency with the protobuf-gradle-plugin/src/main/proto/parmet/buf/test/v1/test.proto b/src/test/resources/FormatCheckWithProtobufGradleTest/format a bad file with a protobuf dependency and a google dependency with the protobuf-gradle-plugin/src/main/proto/parmet/buf/test/v1/test.proto index 2b0127ea..14859b65 100644 --- a/src/test/resources/FormatCheckWithProtobufGradleTest/format a bad file with a protobuf dependency and a google dependency with the protobuf-gradle-plugin/src/main/proto/parmet/buf/test/v1/test.proto +++ b/src/test/resources/FormatCheckWithProtobufGradleTest/format a bad file with a protobuf dependency and a google dependency with the protobuf-gradle-plugin/src/main/proto/parmet/buf/test/v1/test.proto @@ -19,8 +19,10 @@ import "google/protobuf/any.proto"; import "protokt/protokt.proto"; message BasicMessage { - - google.protobuf.Any any = 1; protokt.ProtoktFileOptions protokt_file_options = 2; } + +message Foo { + +} diff --git a/src/test/resources/FormatCheckWithProtobufGradleTest/format a bad separate protobuf source directory through the protobuf-gradle-plugin/subdir/parmet/buf/test/v1/test.proto b/src/test/resources/FormatCheckWithProtobufGradleTest/format a bad separate protobuf source directory through the protobuf-gradle-plugin/subdir/parmet/buf/test/v1/test.proto index 63b1fea9..bad9fd3b 100644 --- a/src/test/resources/FormatCheckWithProtobufGradleTest/format a bad separate protobuf source directory through the protobuf-gradle-plugin/subdir/parmet/buf/test/v1/test.proto +++ b/src/test/resources/FormatCheckWithProtobufGradleTest/format a bad separate protobuf source directory through the protobuf-gradle-plugin/subdir/parmet/buf/test/v1/test.proto @@ -15,7 +15,6 @@ syntax = "proto3"; package parmet.buf.test.v1; -message BasicMessage { - +message Foo { }