Test coverage hardening

ignifyr-common/pom.xml

@@ -15,6 +15,7 @@
 
     <build>
         <sourceDirectory>src/main/scala</sourceDirectory>
+        <testSourceDirectory>src/test/scala</testSourceDirectory>
         <plugins>
             <!-- Allows compiling/testing/running/documenting Scala code in Maven. -->
             <plugin>
@@ -51,6 +52,22 @@
                 <groupId>org.apache.maven.plugins</groupId>
                 <artifactId>maven-jar-plugin</artifactId>
             </plugin>
+            <!-- Short tier only: these helpers are Spark-free and need no container. -->
+            <plugin>
+                <groupId>org.scalatest</groupId>
+                <artifactId>scalatest-maven-plugin</artifactId>
+                <executions>
+                    <execution>
+                        <id>test</id>
+                        <goals>
+                            <goal>test</goal>
+                        </goals>
+                        <configuration>
+                            <wildcardSuites>io.ignifyr.common</wildcardSuites>
+                        </configuration>
+                    </execution>
+                </executions>
+            </plugin>
             <!-- Community edition boundary gate: bans enterprise-only deps (see root pluginManagement). -->
             <plugin>
                 <groupId>org.apache.maven.plugins</groupId>
@@ -79,5 +96,12 @@
             <groupId>io.onfhir</groupId>
             <artifactId>onfhir-definition-commons</artifactId>
         </dependency>
+
+        <!-- For Unit testing -->
+        <dependency>
+            <groupId>org.scalatest</groupId>
+            <artifactId>scalatest_${scala.binary.version}</artifactId>
+            <scope>test</scope>
+        </dependency>
     </dependencies>
 </project>

ignifyr-common/src/test/scala/io/ignifyr/common/app/AppVersionTest.scala

@@ -0,0 +1,17 @@
+package io.ignifyr.common.app
+
+import org.scalatest.flatspec.AnyFlatSpec
+import org.scalatest.matchers.should.Matchers
+
+/**
+ * This module deliberately does not ship `version.properties` — the filtered file lives in
+ * `ignifyr-engine` and `ignifyr-server`. So on this classpath the fallback is the whole behaviour, and
+ * the fallback is what `/metadata` reports when a distribution forgets to filter the resource.
+ */
+class AppVersionTest extends AnyFlatSpec with Matchers {
+
+  "getVersion" should "fall back to UNKNOWN when version.properties is not on the classpath" in {
+    getClass.getClassLoader.getResource("version.properties") shouldBe null
+    AppVersion.getVersion shouldBe "UNKNOWN"
+  }
+}

ignifyr-common/src/test/scala/io/ignifyr/common/util/CustomMappingFunctionsTest.scala

@@ -0,0 +1,47 @@
+package io.ignifyr.common.util
+
+import io.onfhir.path.{FhirPathEvaluator, FhirPathException}
+import org.json4s.JsonAST.JNull
+import org.scalatest.flatspec.AnyFlatSpec
+import org.scalatest.matchers.should.Matchers
+
+/**
+ * The `cst:` FHIRPath library. It is wired by class name only
+ * (`ignifyr.functionLibraries.cst.className`) and that block is **active** in `ignifyr-server`'s
+ * application.conf, so nothing in Scala references it and nothing else would notice a change here.
+ */
+class CustomMappingFunctionsTest extends AnyFlatSpec with Matchers {
+
+  private val evaluator =
+    FhirPathEvaluator
+      .apply()
+      .withDefaultFunctionLibraries()
+      .withFunctionLibrary("cst", new CustomMappingFunctionsFactory())
+
+  /*
+   * createTimeSeriesData base64-*encodes* its input and then reads the encoded bytes back pairwise as
+   * little-endian shorts. For "AB": bytes [65,66] encode to "QUI=" = [81,85,73,61]; the pairs [81,85]
+   * and [73,61] read little-endian are 81 + (85<<8) = 21841 and 73 + (61<<8) = 15689, each widened to
+   * Double before being printed.
+   */
+  "cst:createTimeSeriesData" should "decode the encoded bytes pairwise as little-endian shorts" in {
+    evaluator.evaluateOptionalString("cst:createTimeSeriesData('AB')", JNull) shouldBe Some("21841.0 15689.0")
+  }
+
+  it should "produce one number per two encoded bytes" in {
+    // "ABCDEF" -> 8 base64 characters -> 4 pairs -> 4 numbers.
+    evaluator
+      .evaluateOptionalString("cst:createTimeSeriesData('ABCDEF')", JNull)
+      .map(_.split(" ").length) shouldBe Some(4)
+  }
+
+  it should "be deterministic for the same input" in {
+    val first = evaluator.evaluateOptionalString("cst:createTimeSeriesData('some payload')", JNull)
+    evaluator.evaluateOptionalString("cst:createTimeSeriesData('some payload')", JNull) shouldBe first
+  }
+
+  it should "reject an argument expression that does not return a single string" in {
+    a[FhirPathException] should be thrownBy
+      evaluator.evaluateOptionalString("cst:createTimeSeriesData(42)", JNull)
+  }
+}

ignifyr-common/src/test/scala/io/ignifyr/common/util/ExceptionUtilTest.scala

@@ -0,0 +1,45 @@
+package io.ignifyr.common.util
+
+import org.scalatest.flatspec.AnyFlatSpec
+import org.scalatest.matchers.should.Matchers
+
+/**
+ * `extractExceptionMessages` is what turns a nested failure into the single `description` a
+ * `FhirMappingError` carries, so what it drops is what the user never sees in an execution log.
+ */
+class ExceptionUtilTest extends AnyFlatSpec with Matchers {
+
+  "extractExceptionMessages" should "return the message of an exception with no cause" in {
+    ExceptionUtil.extractExceptionMessages(new RuntimeException("only message")) shouldBe "only message"
+  }
+
+  it should "join the whole cause chain, outermost first" in {
+    val root = new IllegalStateException("root cause")
+    val middle = new IllegalArgumentException("middle cause", root)
+    val top = new RuntimeException("top level", middle)
+    ExceptionUtil.extractExceptionMessages(top) shouldBe "top level\nmiddle cause\nroot cause"
+  }
+
+  it should "skip a null message in the middle of the chain" in {
+    val root = new IllegalStateException("root cause")
+    val middle = new IllegalArgumentException(null: String, root)
+    val top = new RuntimeException("top level", middle)
+    ExceptionUtil.extractExceptionMessages(top) shouldBe "top level\nroot cause"
+  }
+
+  it should "skip an empty message" in {
+    ExceptionUtil.extractExceptionMessages(new RuntimeException("", new IllegalStateException("root"))) shouldBe "root"
+  }
+
+  // Note the explicit null messages: the RuntimeException(Throwable) constructor would otherwise set the
+  // message to the cause's toString, which is exactly the noise this helper is meant to avoid emitting.
+  it should "return an empty string when nothing in the chain has a message" in {
+    val cause = new IllegalStateException(null: String)
+    ExceptionUtil.extractExceptionMessages(new RuntimeException(null: String, cause)) shouldBe ""
+  }
+
+  it should "keep the cause's toString when it was used as the wrapper's message" in {
+    val messages = ExceptionUtil.extractExceptionMessages(new RuntimeException(new IllegalStateException("root")))
+    messages shouldBe "java.lang.IllegalStateException: root\nroot"
+  }
+}

ignifyr-connector-file/src/main/scala/io/ignifyr/connector/file/format/FileFormatRegistry.scala

@@ -38,7 +38,8 @@ object FileFormatRegistry {
       throw MissingFileFormatException(FileFormatHints.describeSourceFormat(contentType))
     )
 
-  private def indexUnique[V](what: String)(entries: Seq[(String, V)]): Map[String, V] = {
+  /** Visible to the connector package so the fail-fast contract can be asserted without a second classloader. */
+  private[file] def indexUnique[V](what: String)(entries: Seq[(String, V)]): Map[String, V] = {
     val byKey = entries.groupBy(_._1).map { case (contentType, group) =>
       if (group.size > 1) {
         val owners = group.map(_._2.getClass.getName).mkString(", ")

ignifyr-connector-file/src/test/resources/file-data-source-reader-test-data/distinct-test/patients-with-duplicates.csv

@@ -0,0 +1,6 @@
+pid,gender,homePostalCode
+p1,male,G02547
+p2,female,H10564
+p1,male,G02547
+p3,male,V13135
+p2,female,H10564

ignifyr-connector-file/src/test/scala/io/ignifyr/connector/file/FileConnectorExtensionSpec.scala

@@ -41,4 +41,26 @@ class FileConnectorExtensionSpec extends AnyFlatSpec with Matchers {
     val ex = intercept[MissingFileFormatException](FileFormatRegistry.sourceFormat(SourceContentTypes.JSON))
     ex.getMessage should include("com.pontegra.ignifyr:ignifyr-format-json")
   }
+
+  // The counterpart of the missing-format path: a content type claimed by *two* installed handlers.
+  // `FileConnectorExtension.initialize` force-materializes this registry so it surfaces at startup
+  // rather than at first read. ServiceLoader input cannot be staged on this classpath, so the guard is
+  // asserted on the indexing helper directly.
+  it should "fail fast naming both handlers when two claim the same content type" in {
+    val csv = FileFormatRegistry.sourceFormat(SourceContentTypes.CSV)
+    val parquet = FileFormatRegistry.sourceFormat(SourceContentTypes.PARQUET)
+    val ex = intercept[IllegalStateException] {
+      FileFormatRegistry.indexUnique("file source format")(Seq("csv" -> csv, "csv" -> parquet))
+    }
+    ex.getMessage should include("Duplicate file source format registration")
+    ex.getMessage should include("csv")
+    ex.getMessage should (include(csv.getClass.getName) and include(parquet.getClass.getName))
+  }
+
+  it should "index one handler per content type" in {
+    val csv = FileFormatRegistry.sourceFormat(SourceContentTypes.CSV)
+    val parquet = FileFormatRegistry.sourceFormat(SourceContentTypes.PARQUET)
+    FileFormatRegistry.indexUnique("file source format")(Seq("a" -> csv, "b" -> parquet)) shouldBe
+      Map("a" -> csv, "b" -> parquet)
+  }
 }

ignifyr-connector-file/src/test/scala/io/ignifyr/connector/file/FileDataSourceReaderOptionsTest.scala

@@ -0,0 +1,106 @@
+package io.ignifyr.connector.file
+
+import io.ignifyr.engine.config.IgnifyrConfig
+import io.ignifyr.engine.model.{FileSystemSource, FileSystemSourceSettings, SourceContentTypes}
+import org.apache.spark.sql.SparkSession
+import org.scalatest.flatspec.AnyFlatSpec
+import org.scalatest.matchers.should.Matchers
+
+import java.nio.file.Paths
+
+/**
+ * The two cross-cutting concerns `FileDataSourceReader` applies around the format handler, neither of
+ * which any other suite reaches: the `distinct` read option, and how a source path is resolved.
+ *
+ * Path resolution has a history worth guarding — an earlier `hdfs://` special case further down the
+ * write side silently turned parquet output into text — so what is pinned here is the one thing the read
+ * side branches on: an `hdfs://` data folder is handed through verbatim, while every other path is
+ * resolved against the workspace folder.
+ */
+class FileDataSourceReaderOptionsTest extends AnyFlatSpec with Matchers {
+
+  private val sparkSession: SparkSession = IgnifyrConfig.sparkSession
+  private val reader = new FileDataSourceReader(sparkSession)
+
+  private val testDataFolderPath: String =
+    Paths.get(getClass.getResource("/file-data-source-reader-test-data").toURI).toAbsolutePath.toString
+
+  private def readDistinctTest(options: Map[String, String]) =
+    reader.read(
+      mappingSourceBinding = FileSystemSource(
+        path = "patients-with-duplicates.csv",
+        contentType = SourceContentTypes.CSV,
+        options = options
+      ),
+      mappingJobSourceSettings = FileSystemSourceSettings(
+        name = "test",
+        sourceUri = "urn:test",
+        dataFolderPath = s"$testDataFolderPath/distinct-test"
+      ),
+      schema = None
+    )
+
+  // The fixture holds 5 rows, of which 2 are exact repeats of earlier ones.
+  "the distinct option" should "drop repeated rows when it is set" in {
+    readDistinctTest(Map("distinct" -> "true")).count() shouldBe 3
+  }
+
+  it should "keep every row when it is absent" in {
+    readDistinctTest(Map.empty).count() shouldBe 5
+  }
+
+  // Only the exact string "true" enables it; anything else reads the file unchanged.
+  it should "keep every row for any value other than true" in {
+    readDistinctTest(Map("distinct" -> "false")).count() shouldBe 5
+    readDistinctTest(Map("distinct" -> "yes")).count() shouldBe 5
+  }
+
+  /*
+   * Path resolution is asserted through the streaming directory check, which reports the *resolved*
+   * path: it runs straight after resolution and needs no Hadoop filesystem, so the branch can be pinned
+   * without an HDFS cluster.
+   */
+  "path resolution" should "hand an hdfs:// data folder through without prefixing the workspace folder" in {
+    val thrown = the[IllegalArgumentException] thrownBy reader.read(
+      mappingSourceBinding = FileSystemSource(path = "patients", contentType = SourceContentTypes.CSV),
+      mappingJobSourceSettings = FileSystemSourceSettings(
+        name = "test",
+        sourceUri = "urn:test",
+        dataFolderPath = "hdfs://namenode:8020/data",
+        asStream = true
+      ),
+      schema = None
+    )
+    thrown.getMessage should startWith("hdfs://namenode:8020/data/patients")
+  }
+
+  it should "join an hdfs:// folder and path with exactly one separator" in {
+    val thrown = the[IllegalArgumentException] thrownBy reader.read(
+      mappingSourceBinding = FileSystemSource(path = "/patients", contentType = SourceContentTypes.CSV),
+      mappingJobSourceSettings = FileSystemSourceSettings(
+        name = "test",
+        sourceUri = "urn:test",
+        dataFolderPath = "hdfs://namenode:8020/data/",
+        asStream = true
+      ),
+      schema = None
+    )
+    thrown.getMessage should startWith("hdfs://namenode:8020/data/patients")
+  }
+
+  it should "resolve a non-hdfs data folder to an absolute local path" in {
+    val thrown = the[IllegalArgumentException] thrownBy reader.read(
+      mappingSourceBinding = FileSystemSource(path = "patients.csv", contentType = SourceContentTypes.CSV),
+      mappingJobSourceSettings = FileSystemSourceSettings(
+        name = "test",
+        sourceUri = "urn:test",
+        dataFolderPath = s"$testDataFolderPath/single-file-test",
+        asStream = true
+      ),
+      schema = None
+    )
+    thrown.getMessage should not startWith "hdfs://"
+    thrown.getMessage should include("patients.csv")
+    thrown.getMessage should include("is not a directory")
+  }
+}