From 1f2d0a2bcf1656c6751d85339dfbcc63e041ad77 Mon Sep 17 00:00:00 2001 From: Brice Jaglin Date: Wed, 20 May 2020 10:24:52 +0200 Subject: [PATCH 1/3] make it possible to attach PrintStream after interface init --- .../scalafix/internal/sbt/ScalafixInterface.scala | 15 ++++++++------- .../scalafix/internal/sbt/ScalafixAPISuite.scala | 4 ++-- 2 files changed, 10 insertions(+), 9 deletions(-) diff --git a/src/main/scala/scalafix/internal/sbt/ScalafixInterface.scala b/src/main/scala/scalafix/internal/sbt/ScalafixInterface.scala index 7d74ef05..aa37f1f9 100644 --- a/src/main/scala/scalafix/internal/sbt/ScalafixInterface.scala +++ b/src/main/scala/scalafix/internal/sbt/ScalafixInterface.scala @@ -1,6 +1,5 @@ package scalafix.internal.sbt -import java.io.PrintStream import java.net.URLClassLoader import java.nio.file.Path import java.{util => jutil} @@ -67,6 +66,11 @@ object Arg { override def apply(sa: ScalafixArguments): ScalafixArguments = sa // caching is currently implemented in sbt-scalafix itself } + + case class PrintStream(printStream: java.io.PrintStream) extends Arg { + override def apply(sa: ScalafixArguments): ScalafixArguments = + sa.withPrintStream(printStream) + } } class ScalafixInterface private ( @@ -86,13 +90,11 @@ class ScalafixInterface private ( private def this( api: ScalafixAPI, toolClasspath: URLClassLoader, - mainCallback: ScalafixMainCallback, - printStream: PrintStream + mainCallback: ScalafixMainCallback ) = this( api .newArguments() .withMainCallback(mainCallback) - .withPrintStream(printStream) .withToolClasspath(toolClasspath), Seq(Arg.ToolClasspath(toolClasspath)) ) @@ -164,8 +166,7 @@ object ScalafixInterface { def fromToolClasspath( scalafixDependencies: Seq[ModuleID], scalafixCustomResolvers: Seq[Repository], - logger: Logger = Compat.ConsoleLogger(System.out), - printStream: PrintStream = System.out + logger: Logger = Compat.ConsoleLogger(System.out) ): () => ScalafixInterface = new LazyValue({ () => val jars = ScalafixCoursier.scalafixCliJars(scalafixCustomResolvers) @@ -175,7 +176,7 @@ object ScalafixInterface { val classloader = new URLClassLoader(urls, interfacesParent) val api = ScalafixAPI.classloadInstance(classloader) val callback = new ScalafixLogger(logger) - new ScalafixInterface(api, classloader, callback, printStream) + new ScalafixInterface(api, classloader, callback) .addToolClasspath( scalafixDependencies, scalafixCustomResolvers, diff --git a/src/test/scala/scalafix/internal/sbt/ScalafixAPISuite.scala b/src/test/scala/scalafix/internal/sbt/ScalafixAPISuite.scala index 995f9205..b658d6b9 100644 --- a/src/test/scala/scalafix/internal/sbt/ScalafixAPISuite.scala +++ b/src/test/scala/scalafix/internal/sbt/ScalafixAPISuite.scala @@ -30,9 +30,9 @@ class ScalafixAPISuite extends AnyFunSuite { .fromToolClasspath( List("com.geirsson" %% "example-scalafix-rule" % "1.3.0"), ScalafixCoursier.defaultResolvers, - logger, - new PrintStream(baos) + logger )() + .withArgs(Arg.PrintStream(new PrintStream(baos))) val tmp = Files.createTempFile("scalafix", "Tmp.scala") tmp.toFile.deleteOnExit() Files.write( From 68923112d44ce0018fa523e8669ac37356f25bef Mon Sep 17 00:00:00 2001 From: Brice Jaglin Date: Wed, 20 May 2020 10:27:15 +0200 Subject: [PATCH 2/3] log `scalafix --check` diffs as errors via sbt logger Diffs were dumped on stdout. When scalafix is run in parallel with many other tasks across multiple sub-projects, this makes it much easier to find out which files are the culprits and why, as the logs stand out and are available via `sbt last scalafix` --- .../internal/sbt/LoggingOutputStream.scala | 43 +++++ .../scala/scalafix/sbt/ScalafixPlugin.scala | 5 +- src/sbt-test/sbt-scalafix/basic/build.sbt | 16 ++ src/sbt-test/sbt-scalafix/basic/test | 1 + .../sbt/LoggingOutputStreamSuite.scala | 147 ++++++++++++++++++ 5 files changed, 211 insertions(+), 1 deletion(-) create mode 100644 src/main/scala/scalafix/internal/sbt/LoggingOutputStream.scala create mode 100644 src/test/scala/scalafix/internal/sbt/LoggingOutputStreamSuite.scala diff --git a/src/main/scala/scalafix/internal/sbt/LoggingOutputStream.scala b/src/main/scala/scalafix/internal/sbt/LoggingOutputStream.scala new file mode 100644 index 00000000..32a031c7 --- /dev/null +++ b/src/main/scala/scalafix/internal/sbt/LoggingOutputStream.scala @@ -0,0 +1,43 @@ +package scalafix.internal.sbt + +import java.io.{ByteArrayOutputStream, OutputStream} + +import sbt.{Level, Logger} + +/** Split an OutputStream into messages and feed them to a given logger at a specified level. Not thread-safe. */ +class LoggingOutputStream( + logger: Logger, + level: Level.Value, + separator: String +) extends OutputStream { + + private val baos = new ByteArrayOutputStream { + def maybeStripSuffix(suffix: Array[Byte]): Option[String] = { + def endsWithSuffix: Boolean = + count >= suffix.length && suffix.zipWithIndex.forall { + case (b: Byte, i: Int) => + b == buf(count - separatorBytes.length + i) + } + + if (endsWithSuffix) + Some(new String(buf, 0, count - separatorBytes.length)) + else None + } + } + + private val separatorBytes = separator.getBytes + require(separatorBytes.length > 0) + + override def write(b: Int): Unit = { + baos.write(b) + baos.maybeStripSuffix(separatorBytes).foreach { message => + logger.log(level, message) + baos.reset() + } + } +} + +object LoggingOutputStream { + def apply(logger: Logger, level: Level.Value): OutputStream = + new LoggingOutputStream(logger, level, System.lineSeparator) +} diff --git a/src/main/scala/scalafix/sbt/ScalafixPlugin.scala b/src/main/scala/scalafix/sbt/ScalafixPlugin.scala index 069ad483..9dec5c20 100644 --- a/src/main/scala/scalafix/sbt/ScalafixPlugin.scala +++ b/src/main/scala/scalafix/sbt/ScalafixPlugin.scala @@ -1,5 +1,6 @@ package scalafix.sbt +import java.io.PrintStream import java.nio.file.{Path, Paths} import com.geirsson.coursiersmall.Repository @@ -168,7 +169,8 @@ object ScalafixPlugin extends AutoPlugin { loadedRules = () => scalafixInterface().availableRules(), terminalWidth = Some(JLineAccess.terminalWidth) ).parser.parsed - + val errorLogger = + new PrintStream(LoggingOutputStream(streams.value.log, Level.Error)) val projectDepsInternal = products.in(ScalafixConfig).value ++ internalDependencyClasspath.in(ScalafixConfig).value.map(_.data) val projectDepsExternal = @@ -192,6 +194,7 @@ object ScalafixPlugin extends AutoPlugin { val mainInterface = mainInterface0 .withArgs(maybeNoCache: _*) .withArgs( + Arg.PrintStream(errorLogger), Arg.Config(scalafixConf), Arg.Rules(shell.rules), Arg.ParsedArgs(shell.extra) diff --git a/src/sbt-test/sbt-scalafix/basic/build.sbt b/src/sbt-test/sbt-scalafix/basic/build.sbt index 1d30344c..2d5dbe29 100644 --- a/src/sbt-test/sbt-scalafix/basic/build.sbt +++ b/src/sbt-test/sbt-scalafix/basic/build.sbt @@ -25,3 +25,19 @@ lazy val example = project ) lazy val tests = project + +lazy val checkLogs = taskKey[Unit]("Check that diffs are logged as errors") + +checkLogs := { + val taskStreams = streams.in(scalafix).in(Compile).in(example).value + val reader = taskStreams.readText(taskStreams.key) + val logLines = Stream + .continually(reader.readLine()) + .takeWhile(_ != null) + .map(_.replaceAll("\u001B\\[[;\\d]*m", "")) // remove control chars (colors) + .force + assert( + logLines.exists(_ == "[error] -import scala.concurrent.Future"), + "diff should be logged as error" + ) +} diff --git a/src/sbt-test/sbt-scalafix/basic/test b/src/sbt-test/sbt-scalafix/basic/test index 2204f1af..6e5bdb0b 100644 --- a/src/sbt-test/sbt-scalafix/basic/test +++ b/src/sbt-test/sbt-scalafix/basic/test @@ -1,4 +1,5 @@ -> example/scalafix --test +> checkLogs > example/scalafix > example/scalafix --test > tests/test diff --git a/src/test/scala/scalafix/internal/sbt/LoggingOutputStreamSuite.scala b/src/test/scala/scalafix/internal/sbt/LoggingOutputStreamSuite.scala new file mode 100644 index 00000000..33662abc --- /dev/null +++ b/src/test/scala/scalafix/internal/sbt/LoggingOutputStreamSuite.scala @@ -0,0 +1,147 @@ +package scalafix.internal.sbt + +import java.io.PrintStream + +import org.scalatest.funsuite.AnyFunSuite +import org.scalatest.matchers.must.Matchers +import sbt.{Level, Logger} + +import scala.collection.mutable + +class LoggingOutputStreamSuite extends AnyFunSuite with Matchers { + + val sep = System.lineSeparator + val CRLF = "\r\n" + + def withStubLogger( + level: Level.Value, + separator: String = sep + )( + testFun: ( + LoggingOutputStream, + mutable.Seq[(Level.Value, String)] + ) => Unit + ): Unit = { + val logs = mutable.ListBuffer[(Level.Value, String)]() + + val logger = new Logger { + override def log(level: Level.Value, message: => String): Unit = + logs += ((level, message)) + + override def success(message: => String): Unit = ??? + override def trace(t: => Throwable): Unit = ??? + } + + testFun(new LoggingOutputStream(logger, level, separator), logs) + } + + test("capture objects printed through PrintStream.println") { + withStubLogger(Level.Warn) { (outputStream, logs) => + val data = 1234 + new PrintStream(outputStream).println(data) + logs must be(mutable.Seq((Level.Warn, String.valueOf(data)))) + } + } + + test("capture messages of increasing length") { + withStubLogger(Level.Warn) { (outputStream, logs) => + val word = "foo" + val printStream = new PrintStream(outputStream) + printStream.println(word) + printStream.println(word * 2) + printStream.println(word * 3) + logs.map(_._2) must be(mutable.Seq(word, word * 2, word * 3)) + } + } + + test("capture messages of decreasing length") { + withStubLogger(Level.Warn) { (outputStream, logs) => + val word = "foo" + val printStream = new PrintStream(outputStream) + printStream.println(word * 3) + printStream.println(word * 2) + printStream.println(word) + logs.map(_._2) must be(mutable.Seq(word * 3, word * 2, word)) + } + } + + test("capture messages of non-monotonic length") { + withStubLogger(Level.Warn) { (outputStream, logs) => + val word = "foo" + val printStream = new PrintStream(outputStream) + printStream.println(word * 3) + printStream.println(word) + printStream.println(word * 2) + logs.map(_._2) must be(mutable.Seq(word * 3, word, word * 2)) + } + } + + test("capture heading empty message") { + withStubLogger(Level.Warn) { (outputStream, logs) => + val message = "hello world!" + outputStream.write(s"$sep$message$sep".getBytes) + logs.map(_._2) must be(mutable.Seq("", message)) + } + } + + test("capture in-between empty message") { + withStubLogger(Level.Warn) { (outputStream, logs) => + val message1 = "hello world!" + val message2 = "here we are" + outputStream.write(s"$message1$sep$sep$message2$sep".stripMargin.getBytes) + logs.map(_._2) must be(mutable.Seq(message1, "", message2)) + } + } + + test("capture trailing empty message") { + withStubLogger(Level.Warn) { (outputStream, logs) => + val message = "hello world!" + outputStream.write(s"$message$sep$sep".getBytes) + logs.map(_._2) must be(mutable.Seq(message, "")) + } + } + + test("capture multi-byte characters") { + withStubLogger(Level.Warn) { (outputStream, logs) => + val messageWithNonAsciiChar = "il était un petit navire" + messageWithNonAsciiChar.getBytes.length must be > messageWithNonAsciiChar.length //assert test is correct + outputStream.write(s"$messageWithNonAsciiChar$sep".getBytes) + logs.map(_._2) must be(mutable.Seq(messageWithNonAsciiChar)) + } + } + + test("handle multi-character separator") { + withStubLogger(Level.Warn, CRLF) { (outputStream, logs) => + val message1 = "hello world!" + val message2 = "here we are" + outputStream.write(s"$message1$CRLF$CRLF$message2$CRLF".getBytes) + logs.map(_._2) must be(mutable.Seq(message1, "", message2)) + } + } + + test("capture very long messages") { + withStubLogger(Level.Warn) { (outputStream, logs) => + val veryLongMessage = "a" * 1000000 // this would exhaust memory on quadratic implementations + outputStream.write(s"$veryLongMessage$sep".getBytes) + logs.map(_._2) must be(mutable.Seq(veryLongMessage)) + } + } + + test( + "capture very long messages containing a subset of the line separator" + ) { + withStubLogger(Level.Warn, CRLF) { (outputStream, logs) => + val veryLongMessage = CRLF.head.toString * 1000000 + outputStream.write(s"$veryLongMessage$CRLF".getBytes) + logs.map(_._2) must be(mutable.Seq(veryLongMessage)) + } + } + + test("fail verbosely for invalid separator") { + an[IllegalArgumentException] should be thrownBy new LoggingOutputStream( + Logger.Null, + Level.Warn, + separator = "" + ) + } +} From 13efb642f227525ccc81b5a0b51d4e91a44b4ecb Mon Sep 17 00:00:00 2001 From: Brice Jaglin Date: Wed, 20 May 2020 11:30:51 +0200 Subject: [PATCH 3/3] fix transient failures by adding some delay before reading logs --- src/sbt-test/sbt-scalafix/basic/test | 1 + 1 file changed, 1 insertion(+) diff --git a/src/sbt-test/sbt-scalafix/basic/test b/src/sbt-test/sbt-scalafix/basic/test index 6e5bdb0b..06da863e 100644 --- a/src/sbt-test/sbt-scalafix/basic/test +++ b/src/sbt-test/sbt-scalafix/basic/test @@ -1,4 +1,5 @@ -> example/scalafix --test +$ sleep 2000 > checkLogs > example/scalafix > example/scalafix --test