[GitHub] [daffodil] stevedlawrence commented on a diff in pull request #965: Format test and sbt files

["stevedlawrence (via GitHub)" <gi...@apache.org>]

stevedlawrence commented on code in PR #965:
URL: https://github.com/apache/daffodil/pull/965#discussion_r1107169053


##########
daffodil-cli/src/test/scala/org/apache/daffodil/cli/cliTest/TestBlob.scala:
##########
@@ -110,19 +111,23 @@ class TestBlob {
    *
    ***/
   @Test def test_2GB_blob(): Unit = {
-    val schema = path("daffodil-cli/src/test/resources/org/apache/daffodil/cli/large_blob.dfdl.xsd")
+    val schema = path(
+      "daffodil-cli/src/test/resources/org/apache/daffodil/cli/large_blob.dfdl.xsd",
+    )
     val input = path("daffodil-cli/src/test/resources/org/apache/daffodil/cli/input/2049MB.bin")
 
     assumeTrue("large test input file must be manually generated", exists(input))
 
     withTempFile { infoset =>
       withTempFile { unparse =>
         withBlobDir {
-          runCLI(args"parse -s $schema -o $infoset $input", timeout = 120) { cli =>
-          } (ExitCode.Success)
+          runCLI(args"parse -s $schema -o $infoset $input", timeout = 120) { cli => }(
+            ExitCode.Success,
+          )
 
-          runCLI(args"unparse -s $schema -o $unparse $infoset", timeout = 120) { cli =>
-          } (ExitCode.Success)
+          runCLI(args"unparse -s $schema -o $unparse $infoset", timeout = 120) { cli => }(
+            ExitCode.Success,
+          )

Review Comment:
   This particular changes makes this much less readable. For consistency, we really want all these tests to have the same pattern of
   
   ```scala
   runCLI(args"...") { cli =>
     // zero or more lines of actual checks
   }(ExitCode.Foo)
   ```
   
   It seems the ones with bad formatting are those without actual CLI expect checks, likely because all the test cares about is the exit code. I don't think there are too many of those--maybe we just add a comment that makes it clear no expect logic is intentional, and it might also prevent scalafmt from changing these lines? For example:
   
   ```scala
   runCLI(args"...") { cli =>
     // this block intentionally left blank
   }(ExitCode.Success)
   ```



##########
daffodil-cli/src/test/scala/org/apache/daffodil/cli/cliTest/TestBlob.scala:
##########
@@ -110,19 +111,23 @@ class TestBlob {
    *
    ***/
   @Test def test_2GB_blob(): Unit = {
-    val schema = path("daffodil-cli/src/test/resources/org/apache/daffodil/cli/large_blob.dfdl.xsd")
+    val schema = path(
+      "daffodil-cli/src/test/resources/org/apache/daffodil/cli/large_blob.dfdl.xsd",
+    )

Review Comment:
   I don't really like how this is changing all these one liner `path` calls to 3 lines. I guess they're longer than our max line length, but it isn't really saving much except making it a bit harder to read, IMO. Not sure if there's much we can do about it though.



##########
daffodil-cli/src/test/scala/org/apache/daffodil/cli/cliTest/schematron/TestSvrlOutput.scala:
##########
@@ -17,55 +17,64 @@
 
 package org.apache.daffodil.cliTest.schematron
 
+import java.nio.charset.StandardCharsets.UTF_8
 import java.nio.file.Files
 import java.nio.file.Path
 import java.nio.file.StandardOpenOption.APPEND
-import java.nio.charset.StandardCharsets.UTF_8
+import scala.xml.XML
+
+import org.apache.daffodil.cli.Main.ExitCode
+import org.apache.daffodil.cli.cliTest.Util._
 
 import org.junit.Assert.assertFalse
 import org.junit.Assert.assertTrue
 import org.junit.Assert.fail
 import org.junit.Test
 
-import scala.xml.XML
-
-import org.apache.daffodil.cli.cliTest.Util._
-import org.apache.daffodil.cli.Main.ExitCode
-
 class TestSvrlOutput {
 
   private def makeConf(conf: Path, schematron: Path, svrl: Path): Unit = {
-    Files.write(conf, s"""schematron.path="${jsonEscape(schematron.toString)}"\n""".getBytes(UTF_8), APPEND)
-    Files.write(conf, s"""schematron.svrl.file="${jsonEscape(svrl.toString)}"\n""".getBytes(UTF_8), APPEND)
+    Files.write(
+      conf,
+      s"""schematron.path="${jsonEscape(schematron.toString)}"\n""".getBytes(UTF_8),
+      APPEND,
+    )
+    Files.write(
+      conf,
+      s"""schematron.svrl.file="${jsonEscape(svrl.toString)}"\n""".getBytes(UTF_8),
+      APPEND,
+    )
   }
 
   @Test def validationSuccess(): Unit = {
     val schema = path("daffodil-schematron/src/test/resources/xsd/string.dfdl.xsd")
     val schematron = path("daffodil-schematron/src/test/resources/sch/never-fails.sch")
     val input = path("daffodil-cli/src/test/resources/org/apache/daffodil/cli/input/uuid.txt")
 
-    withTempFile(".conf", { conf =>
-      withTempFile { svrl =>
-
-        makeConf(conf, schematron, svrl)
-
-        runCLI(args"parse --validate schematron=$conf -s $schema $input") { cli =>
-          cli.expect("<never-fails>2f6481e6-542c-11eb-ae93-0242ac130002</never-fails>")
-        } (ExitCode.Success)
-
-        XML.loadFile(svrl.toFile) match {
-          case <svrl:schematron-output>{rules @ _*}</svrl:schematron-output> =>
-            val res = rules.find {
-              case <svrl:failed-assert>{  _* }</svrl:failed-assert> => true
-              case _ => false
-            }
-            // we should not have found failures
-            assertFalse(res.isDefined)
-          case _ =>
-            fail("schematron pattern didnt match")
+    withTempFile(
+      ".conf",
+      { conf =>

Review Comment:
   I don't even understand why it made this change.



##########
daffodil-cli/src/test/scala/org/apache/daffodil/cli/cliTest/schematron/TestEmbedded.scala:
##########
@@ -40,99 +41,111 @@ class TestEmbedded {
     runCLI(args"""parse -r list -s $schema""") { cli =>
       cli.send("widget,monday,1,$5.00,$5.00", inputDone = true)
       cli.expect("</ex:list>")
-    } (ExitCode.Success)
+    }(ExitCode.Success)
   }
 
   @Test def unitPriceWithValidation(): Unit = {
     val schema = path("daffodil-schematron/src/test/resources/xsd/unit_price.dfdl.xsd")
 
-    runCLI(args"""parse -r list --validate schematron="${jsonEscape(schema.toString)}" -s $schema""") { cli =>
+    runCLI(
+      args"""parse -r list --validate schematron="${jsonEscape(schema.toString)}" -s $schema""",
+    ) { cli =>
       cli.send("widget,monday,1,$5.00,$6.00", inputDone = true)
       cli.expect("</ex:list>")
       cli.expectErr("Validation Error: wrong unit price for widget, monday")
-    } (ExitCode.ParseError)
+    }(ExitCode.ParseError)
   }
 
   @Test def unitPriceWithValidationCheckMessage(): Unit = {
     val schema = path("daffodil-schematron/src/test/resources/xsd/unit_price.dfdl.xsd")
 
-    runCLI(args"""parse -r list --validate schematron="${jsonEscape(schema.toString)}" -s $schema""") { cli =>
+    runCLI(
+      args"""parse -r list --validate schematron="${jsonEscape(schema.toString)}" -s $schema""",
+    ) { cli =>
       cli.send("widget,monday,5,$5.00,$25.00||gadget,tuesday,1,$10.00,$11.00", inputDone = true)
       cli.expect("</ex:list>")
       cli.expectErr("Validation Error: wrong unit price for gadget, tuesday")
-    } (ExitCode.ParseError)
+    }(ExitCode.ParseError)
   }
 
   @Test def extends1(): Unit = {
     val schema = path("daffodil-schematron/src/test/resources/xsd/extends-1.dfdl.xsd")
 
-    runCLI(args"""parse --validate schematron="${jsonEscape(schema.toString)}" -s $schema""") { cli =>
-      cli.send("bob;l;smith", inputDone = true)
-      cli.expect("</name>")
-    } (ExitCode.Success) 
+    runCLI(args"""parse --validate schematron="${jsonEscape(schema.toString)}" -s $schema""") {
+      cli =>
+        cli.send("bob;l;smith", inputDone = true)
+        cli.expect("</name>")
+    }(ExitCode.Success)
   }
 
   @Test def extends2(): Unit = {
     val schema = path("daffodil-schematron/src/test/resources/xsd/extends-1.dfdl.xsd")
 
-    runCLI(args"""parse --validate schematron="${jsonEscape(schema.toString)}" -s $schema""") { cli =>
-      cli.send("ob;;smith", inputDone = true)
-      cli.expect("</name>")
-    } (ExitCode.Success) 
+    runCLI(args"""parse --validate schematron="${jsonEscape(schema.toString)}" -s $schema""") {
+      cli =>
+        cli.send("ob;;smith", inputDone = true)
+        cli.expect("</name>")
+    }(ExitCode.Success)
   }
 
   @Test def extends3(): Unit = {
     val schema = path("daffodil-schematron/src/test/resources/xsd/extends-1.dfdl.xsd")
 
-    runCLI(args"""parse --validate schematron="${jsonEscape(schema.toString)}" -s $schema""") { cli =>
-      cli.send(";;smith", inputDone = true)
-      cli.expectErr("Validation Error: first is blank")
-    } (ExitCode.ParseError)
+    runCLI(args"""parse --validate schematron="${jsonEscape(schema.toString)}" -s $schema""") {
+      cli =>
+        cli.send(";;smith", inputDone = true)
+        cli.expectErr("Validation Error: first is blank")
+    }(ExitCode.ParseError)
   }
 
   @Test def extends4(): Unit = {
     val schema = path("daffodil-schematron/src/test/resources/xsd/extends-1.dfdl.xsd")
 
-    runCLI(args"""parse --validate schematron="${jsonEscape(schema.toString)}" -s $schema""") { cli =>
-      cli.send("bob;l;", inputDone = true)
-      cli.expectErr("Validation Error: last is blank")
-    } (ExitCode.ParseError)
+    runCLI(args"""parse --validate schematron="${jsonEscape(schema.toString)}" -s $schema""") {
+      cli =>
+        cli.send("bob;l;", inputDone = true)
+        cli.expectErr("Validation Error: last is blank")
+    }(ExitCode.ParseError)
   }
 
   @Test def extends5(): Unit = {
     val schema = path("daffodil-schematron/src/test/resources/xsd/extends-1.dfdl.xsd")
 
-    runCLI(args"""parse --validate schematron="${jsonEscape(schema.toString)}" -s $schema""") { cli =>
-      cli.send(";l;", inputDone = true)
-      cli.expectErr("Validation Error: last is blank")
-      cli.expectErr("Validation Error: first is blank")
-    } (ExitCode.ParseError)
+    runCLI(args"""parse --validate schematron="${jsonEscape(schema.toString)}" -s $schema""") {
+      cli =>
+        cli.send(";l;", inputDone = true)
+        cli.expectErr("Validation Error: last is blank")
+        cli.expectErr("Validation Error: first is blank")
+    }(ExitCode.ParseError)
   }
 
   @Test def testWithNs1(): Unit = {
     val schema = path("daffodil-schematron/src/test/resources/xsd/with-ns-1.dfdl.xsd")
 
-    runCLI(args"""parse --validate schematron="${jsonEscape(schema.toString)}" -s $schema""") { cli =>
-      cli.send("0;1", inputDone = true)
-      cli.expect("</myns:interval>")
-    } (ExitCode.Success)
+    runCLI(args"""parse --validate schematron="${jsonEscape(schema.toString)}" -s $schema""") {
+      cli =>
+        cli.send("0;1", inputDone = true)
+        cli.expect("</myns:interval>")
+    }(ExitCode.Success)
   }
 
   @Test def testWithNs2(): Unit = {
     val schema = path("daffodil-schematron/src/test/resources/xsd/with-ns-1.dfdl.xsd")
 
-    runCLI(args"""parse --validate schematron="${jsonEscape(schema.toString)}" -s $schema""") { cli =>
-      cli.send("2;1", inputDone = true)
-      cli.expectErr("Validation Error")
-    } (ExitCode.ParseError)
+    runCLI(args"""parse --validate schematron="${jsonEscape(schema.toString)}" -s $schema""") {
+      cli =>

Review Comment:
   I think this is another example about this being very aggressive about line lengths which ultimately hurts readability.



##########
project/Rat.scala:
##########
@@ -101,12 +112,24 @@ object Rat {
     file("daffodil-tdml-lib/src/test/resources/test/tdml/test.txt"),
     file("daffodil-tdml-processor/src/test/resources/test/tdml/test.bin"),
     file("daffodil-tdml-processor/src/test/resources/test/tdml/test.txt"),
-    file("daffodil-test/src/test/resources/org/apache/daffodil/runtime2/ISRM_green_to_orange_60000.0.dat"),
-    file("daffodil-test/src/test/resources/org/apache/daffodil/runtime2/ISRM_green_to_orange_60000.1.dat"),
-    file("daffodil-test/src/test/resources/org/apache/daffodil/runtime2/ISRM_orange_to_green_60002.dat"),
-    file("daffodil-test/src/test/resources/org/apache/daffodil/runtime2/MPU_green_to_orange_60004.dat"),
-    file("daffodil-test/src/test/resources/org/apache/daffodil/runtime2/MPU_orange_to_green_60006.0.dat"),
-    file("daffodil-test/src/test/resources/org/apache/daffodil/runtime2/MPU_orange_to_green_60006.1.dat"),
+    file(
+      "daffodil-test/src/test/resources/org/apache/daffodil/runtime2/ISRM_green_to_orange_60000.0.dat",
+    ),
+    file(
+      "daffodil-test/src/test/resources/org/apache/daffodil/runtime2/ISRM_green_to_orange_60000.1.dat",
+    ),
+    file(
+      "daffodil-test/src/test/resources/org/apache/daffodil/runtime2/ISRM_orange_to_green_60002.dat",
+    ),
+    file(
+      "daffodil-test/src/test/resources/org/apache/daffodil/runtime2/MPU_green_to_orange_60004.dat",
+    ),
+    file(
+      "daffodil-test/src/test/resources/org/apache/daffodil/runtime2/MPU_orange_to_green_60006.0.dat",
+    ),
+    file(
+      "daffodil-test/src/test/resources/org/apache/daffodil/runtime2/MPU_orange_to_green_60006.1.dat",
+    ),

Review Comment:
   Another example where long lines really should be allowed. This changed 1 line that's over the line length limit, to two short lines and a third line that's still over the limit, just a bit shorter.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@daffodil.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org