From 929b3180cc099ba76859f5e88710d2ac087fbfa3 Mon Sep 17 00:00:00 2001 From: Samir Talwar Date: Thu, 30 Jan 2020 10:34:10 +0100 Subject: [PATCH 1/5] When building JMH benchmarks, fail on error, instead of proceeding. Currently, if there are errors in some (but not all) benchmarks, running `bazel run //path/to/benchmark` will compile them, fail on some, and then run the rest. This changes that behavior so that any JMH build failure will fail the build. --- .../jmh_support/BenchmarkGenerator.scala | 29 ++++++++++++------- 1 file changed, 18 insertions(+), 11 deletions(-) diff --git a/src/scala/io/bazel/rules_scala/jmh_support/BenchmarkGenerator.scala b/src/scala/io/bazel/rules_scala/jmh_support/BenchmarkGenerator.scala index d367ff424..b443f3e68 100644 --- a/src/scala/io/bazel/rules_scala/jmh_support/BenchmarkGenerator.scala +++ b/src/scala/io/bazel/rules_scala/jmh_support/BenchmarkGenerator.scala @@ -41,15 +41,24 @@ object BenchmarkGenerator { classPath: List[Path] ) + private case class GenerationException(messageLines: Seq[String]) + extends RuntimeException(messageLines.mkString("\n")) + def main(argv: Array[String]): Unit = { val args = parseArgs(argv) - generateJmhBenchmark( - args.generatorType, - args.resultSourceJar, - args.resultResourceJar, - args.inputJar, - args.classPath - ) + try { + generateJmhBenchmark( + args.generatorType, + args.resultSourceJar, + args.resultResourceJar, + args.inputJar, + args.classPath + ) + } catch { + case GenerationException(messageLines) => + messageLines.foreach(log) + sys.exit(1) + } } private def parseArgs(argv: Array[String]): BenchmarkGeneratorArgs = { @@ -168,10 +177,8 @@ object BenchmarkGenerator { generator.generate(source, destination) generator.complete(source, destination) if (destination.hasErrors) { - log("JMH Benchmark generator failed") - for (e <- destination.getErrors.asScala) { - log(e.toString) - } + throw new GenerationException( + "JHM Benchmark generator failed" +: destination.getErrors.asScala.map(_.toString).toSeq) } } constructJar(sourceJarOut, tmpSourceDir) From c457d3a8e15652e9822e38a0799348a38e05f0e1 Mon Sep 17 00:00:00 2001 From: Samir Talwar Date: Fri, 31 Jan 2020 16:26:35 +0100 Subject: [PATCH 2/5] Add a test for expected failures in JMH benchmarks. --- test/shell/test_misc.sh | 13 +++++++++++-- test_expect_failure/jmh/BUILD | 12 ++++++++++++ test_expect_failure/jmh/InvalidBenchmark.scala | 10 ++++++++++ test_expect_failure/jmh/ValidBenchmark.scala | 9 +++++++++ 4 files changed, 42 insertions(+), 2 deletions(-) create mode 100644 test_expect_failure/jmh/BUILD create mode 100644 test_expect_failure/jmh/InvalidBenchmark.scala create mode 100644 test_expect_failure/jmh/ValidBenchmark.scala diff --git a/test/shell/test_misc.sh b/test/shell/test_misc.sh index e1a767b71..fa23e722b 100755 --- a/test/shell/test_misc.sh +++ b/test/shell/test_misc.sh @@ -55,12 +55,21 @@ test_repl() { test_benchmark_jmh() { RES=$(bazel run -- test/jmh:test_benchmark -i1 -f1 -wi 1) - RESPONSE_CODE=$? + if [ $? -ne 0 ]; then + exit 1 + fi if [[ $RES != *Result*Benchmark* ]]; then echo "Benchmark did not produce expected output:\n$RES" exit 1 fi - exit $RESPONSE_CODE + + set +e + + bazel build test_expect_failure/jmh:jmh_reports_failure + if [ $? -eq 0 ]; then + echo "'bazel build test_expect_failure/jmh:jmh_reports_failure' should have failed." + exit 1 + fi } scala_test_test_filters() { diff --git a/test_expect_failure/jmh/BUILD b/test_expect_failure/jmh/BUILD new file mode 100644 index 000000000..a072803bb --- /dev/null +++ b/test_expect_failure/jmh/BUILD @@ -0,0 +1,12 @@ +load( + "//jmh:jmh.bzl", + "scala_benchmark_jmh", +) + +scala_benchmark_jmh( + name = "jmh_reports_failure", + srcs = [ + "InvalidBenchmark.scala", + "ValidBenchmark.scala", + ], +) diff --git a/test_expect_failure/jmh/InvalidBenchmark.scala b/test_expect_failure/jmh/InvalidBenchmark.scala new file mode 100644 index 000000000..5a0472942 --- /dev/null +++ b/test_expect_failure/jmh/InvalidBenchmark.scala @@ -0,0 +1,10 @@ +package foo + +import org.openjdk.jmh.annotations.Benchmark + +// Benchmark classes cannot be final. +final class InvalidBenchmark { + @Benchmark + def sumIntegersBenchmark: Int = + (1 to 100).sum +} diff --git a/test_expect_failure/jmh/ValidBenchmark.scala b/test_expect_failure/jmh/ValidBenchmark.scala new file mode 100644 index 000000000..42ef7f2ab --- /dev/null +++ b/test_expect_failure/jmh/ValidBenchmark.scala @@ -0,0 +1,9 @@ +package foo + +import org.openjdk.jmh.annotations.Benchmark + +class ValidBenchmark { + @Benchmark + def sumIntegersBenchmark: Int = + (1 to 100).sum +} From 2287da0792b2dff440cff76cbfc2dfdb298382a2 Mon Sep 17 00:00:00 2001 From: Samir Talwar Date: Fri, 31 Jan 2020 16:31:25 +0100 Subject: [PATCH 3/5] Document the fix to JMH builds as a breaking change. --- README.md | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/README.md b/README.md index 38c742cf1..7570340e6 100644 --- a/README.md +++ b/README.md @@ -123,6 +123,12 @@ for an example workspace using another scala version. | 0.14.x | 3b9ab9be31ac217d3337c709cb6bfeb89c8dcbb1 | | 0.13.x | 3c987b6ae8a453886759b132f1572c0efca2eca2 | +## Breaking changes + +If you're upgrading to a version containing one of these commits, you may encounter a breaking change where there was previously undefined behavior. + +- [929b318](https://github.com/bazelbuild/rules_scala/commit/929b3180cc099ba76859f5e88710d2ac087fbfa3) on 2020-01-30: Fixed a bug in the JMH benchmark build that was allowing build failures to creep through. Previously you were able to build a benchmark suite with JMH build errors. Running the benchmark suite would only run the successfully-built benchmarks. + ## Usage with [bazel-deps](https://github.com/johnynek/bazel-deps) Bazel-deps allows you to generate bazel dependencies transitively for maven artifacts. Generally we don't want bazel-deps to fetch From a75c8177885fdaabc93487619e950a8077f550a5 Mon Sep 17 00:00:00 2001 From: Samir Talwar Date: Mon, 3 Feb 2020 11:26:59 +0100 Subject: [PATCH 4/5] Split "test_benchmark_jmh_failure" from "test_benchmark_jmh". --- test/shell/test_misc.sh | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/test/shell/test_misc.sh b/test/shell/test_misc.sh index fa23e722b..575f9bc2e 100755 --- a/test/shell/test_misc.sh +++ b/test/shell/test_misc.sh @@ -63,6 +63,10 @@ test_benchmark_jmh() { exit 1 fi + exit 0 +} + +test_benchmark_jmh_failure() { set +e bazel build test_expect_failure/jmh:jmh_reports_failure @@ -70,6 +74,8 @@ test_benchmark_jmh() { echo "'bazel build test_expect_failure/jmh:jmh_reports_failure' should have failed." exit 1 fi + + exit 0 } scala_test_test_filters() { @@ -126,6 +132,7 @@ $runner test_disappearing_class $runner test_transitive_deps $runner test_repl $runner test_benchmark_jmh +$runner test_benchmark_jmh_failure $runner scala_test_test_filters $runner test_multi_service_manifest $runner test_override_javabin From fb304607fb606ed069db6561db8a68f675afccca Mon Sep 17 00:00:00 2001 From: Samir Talwar Date: Mon, 3 Feb 2020 11:27:22 +0100 Subject: [PATCH 5/5] We don't need ValidBenchmark.scala when testing JMH failures. --- test_expect_failure/jmh/BUILD | 1 - test_expect_failure/jmh/ValidBenchmark.scala | 9 --------- 2 files changed, 10 deletions(-) delete mode 100644 test_expect_failure/jmh/ValidBenchmark.scala diff --git a/test_expect_failure/jmh/BUILD b/test_expect_failure/jmh/BUILD index a072803bb..fbd48091d 100644 --- a/test_expect_failure/jmh/BUILD +++ b/test_expect_failure/jmh/BUILD @@ -7,6 +7,5 @@ scala_benchmark_jmh( name = "jmh_reports_failure", srcs = [ "InvalidBenchmark.scala", - "ValidBenchmark.scala", ], ) diff --git a/test_expect_failure/jmh/ValidBenchmark.scala b/test_expect_failure/jmh/ValidBenchmark.scala deleted file mode 100644 index 42ef7f2ab..000000000 --- a/test_expect_failure/jmh/ValidBenchmark.scala +++ /dev/null @@ -1,9 +0,0 @@ -package foo - -import org.openjdk.jmh.annotations.Benchmark - -class ValidBenchmark { - @Benchmark - def sumIntegersBenchmark: Int = - (1 to 100).sum -}