This is an automated email from the ASF dual-hosted git repository. github-merge-queue[bot] pushed a commit to branch gh-readonly-queue/main/pr-7540-af38ca99c13c0be4fbe69cc168c82d69aae36c56 in repository https://gitbox.apache.org/repos/asf/texera.git
commit 0414dc2c851d2ce591a3d4b9a65a728aa4fb50ba Author: Meng Wang <[email protected]> AuthorDate: Mon Aug 10 23:42:40 2026 -0700 test(amber): cover version-importance helpers and the email message builder (#7540) ### What changes were proposed in this PR? Covers the remaining unit-testable pure logic in two of the three classes the issue lists. No production code was changed. **`WorkflowVersionResource`** (+7 tests) — the version-importance helpers, all private and exercised through `PrivateMethodTester`: - `isSnapshotImportant` — a patch whose ops are all `replace` is unimportant, any other op makes it important, and an empty patch is unimportant. - `isVersionImportant` — a patch touching only `/operatorPositions/` is unimportant; anything else is important. - `isWithinTimeLimit` — holds inside the aggregate window, not outside it. - `encodeVersionImportance` — the latest version is always important, a version inside the aggregate window is folded in as unimportant, and one outside it is judged on its content. > The issue calls this gap `jsonTreeIterator`; that is a local variable inside > `isSnapshotImportant` / `isVersionImportant` rather than a method, so the tests > target those two methods (plus the rest of the family). **`WorkflowEmailNotifier`** (+2 tests) - `createEmailMessage` — the assembled message addresses the recipient and carries the same subject the dedicated builder produces, plus the workflow name, id and state in its content. - `sendStatusEmail` — the invalid-recipient arm returns before dispatching, so the test never reaches `GmailResource.sendEmail`. After this change (measured with `sbt WorkflowExecutionService/jacoco`): `WorkflowVersionResource` 116/121 lines (95%), `WorkflowEmailNotifier` 40/43 (93%). The lines that remain are not unit-reachable: - `updateLatestVersion` — defined but has no callers anywhere in the repo. - `sendStatusEmail`'s dispatch arm — it calls `GmailResource.sendEmail`, i.e. it would send real mail. - `cloneVersion`'s catch block — needs a database-layer failure to trigger. - `isSnapshotInRangeUnimportant`'s `lowerBound == UpperBound` early return is already asserted by an existing test, but jacoco still reports it unhit: the parameters are `java.lang.Integer`, so `==` compares references rather than values. Worth a look separately — this PR does not change production code. **`ResultExportService`** — no tests added. Its in-scope pure logic is already covered by the existing spec: `parseOperators` (valid / empty / malformed), `validateExportRequest` (both arms), `convertFieldToBytes` (all three cases), `generateFileName` (parquet→zip, path-separator stripping), `streamCellData` (all three guards), and the `errorMessages` accumulation the issue mentions (`exportToDataset` "collect one message per operator" / "turn a thrown per-operator failure into an error entry"). The `download.dat` default the issue asks for is unreachable: when `exportOperatorResultAsStream` returns no file name it also returns a null stream, and the preceding line throws. The remaining uncovered methods (`exportOperatorsAsZip`, `exportSingleOperatorToDataset`, `getOperatorDocument`) all go through `DocumentFactory`/storage, which the issue places out of scope. ### Any related issues, documentation, discussions? Closes #7537 ### How was this PR tested? Unit tests, run locally against embedded Postgres (`MockTexeraDB`). All pass, and the failure path was verified by breaking an assertion to confirm the suite goes red: ``` sbt "WorkflowExecutionService/testOnly *WorkflowVersionResourceSpec *WorkflowEmailNotifierSpec" # Tests: succeeded 37, failed 0 sbt "WorkflowExecutionService/Test/scalafmtCheck" # clean sbt "WorkflowExecutionService/Test/scalafix --check" # clean ``` ### Was this PR authored or co-authored using generative AI tooling? Generated-by: Claude Code (Opus 4.8 [1M context]) --- .../workflow/WorkflowVersionResourceSpec.scala | 73 +++++++++++++++++++++- .../web/service/WorkflowEmailNotifierSpec.scala | 33 ++++++++++ 2 files changed, 105 insertions(+), 1 deletion(-) diff --git a/amber/src/test/scala/org/apache/texera/web/resource/dashboard/user/workflow/WorkflowVersionResourceSpec.scala b/amber/src/test/scala/org/apache/texera/web/resource/dashboard/user/workflow/WorkflowVersionResourceSpec.scala index b633ee83a4..15a3bb6654 100644 --- a/amber/src/test/scala/org/apache/texera/web/resource/dashboard/user/workflow/WorkflowVersionResourceSpec.scala +++ b/amber/src/test/scala/org/apache/texera/web/resource/dashboard/user/workflow/WorkflowVersionResourceSpec.scala @@ -41,7 +41,7 @@ import org.apache.texera.dao.jooq.generated.tables.pojos.{ import org.jooq.impl.DSL import org.scalatest.flatspec.AnyFlatSpec import org.scalatest.matchers.should.Matchers -import org.scalatest.{BeforeAndAfterAll, BeforeAndAfterEach} +import org.scalatest.{BeforeAndAfterAll, BeforeAndAfterEach, PrivateMethodTester} import java.sql.Timestamp import java.util.UUID @@ -55,6 +55,7 @@ class WorkflowVersionResourceSpec with Matchers with BeforeAndAfterAll with BeforeAndAfterEach + with PrivateMethodTester with MockTexeraDB { private val testWorkflowWid = 2000 + scala.util.Random.nextInt(1000) @@ -453,4 +454,74 @@ class WorkflowVersionResourceSpec cloned should not be null cloned.getName should include("_copy") } + + // ─── version-importance helpers (pure JSON/timestamp logic) ──────────────── + + private val isSnapshotImportant = PrivateMethod[Boolean](Symbol("isSnapshotImportant")) + private val isVersionImportant = PrivateMethod[Boolean](Symbol("isVersionImportant")) + private val isWithinTimeLimit = PrivateMethod[Boolean](Symbol("isWithinTimeLimit")) + private val encodeVersionImportance = + PrivateMethod[List[WorkflowVersionResource.VersionEntry]](Symbol("encodeVersionImportance")) + + "isSnapshotImportant" should "treat a patch whose ops are all 'replace' as unimportant" in { + val content = """[{"op":"replace","path":"/operatorPositions/x","value":1}]""" + (WorkflowVersionResource invokePrivate isSnapshotImportant(content)) shouldBe false + } + + it should "treat a patch containing any non-replace op as important" in { + val content = """[{"op":"replace","path":"/a"},{"op":"add","path":"/operators/0"}]""" + (WorkflowVersionResource invokePrivate isSnapshotImportant(content)) shouldBe true + } + + it should "treat an empty patch as unimportant" in { + (WorkflowVersionResource invokePrivate isSnapshotImportant("[]")) shouldBe false + } + + "isVersionImportant" should "treat a patch touching only operator positions as unimportant" in { + val content = """[{"op":"replace","path":"/operatorPositions/op-1/x","value":5}]""" + (WorkflowVersionResource invokePrivate isVersionImportant(content)) shouldBe false + } + + it should "treat a patch touching anything else as important" in { + val content = """[{"op":"replace","path":"/operators/0/properties","value":{}}]""" + (WorkflowVersionResource invokePrivate isVersionImportant(content)) shouldBe true + } + + "isWithinTimeLimit" should "hold for timestamps closer together than the aggregate limit" in { + val later = new Timestamp(1_700_000_000_000L) + val earlier = new Timestamp(later.getTime - TimeUnit.SECONDS.toMillis(1)) + (WorkflowVersionResource invokePrivate isWithinTimeLimit(later, earlier)) shouldBe true + } + + it should "not hold once the gap exceeds the aggregate limit" in { + val later = new Timestamp(1_700_000_000_000L) + val earlier = new Timestamp(later.getTime - TimeUnit.DAYS.toMillis(30)) + (WorkflowVersionResource invokePrivate isWithinTimeLimit(later, earlier)) shouldBe false + } + + "encodeVersionImportance" should "always mark the latest version important and aggregate close ones" in { + val base = 1_700_000_000_000L + def version(vid: Int, offsetMillis: Long, content: String): WorkflowVersion = { + val v = new WorkflowVersion + v.setVid(vid) + v.setWid(testWorkflowWid) + v.setContent(content) + v.setCreationTime(new Timestamp(base - offsetMillis)) + v + } + + val positional = """[{"op":"replace","path":"/operatorPositions/op-1/x","value":5}]""" + val meaningful = """[{"op":"replace","path":"/operators/0/properties","value":{}}]""" + + val encoded = WorkflowVersionResource invokePrivate encodeVersionImportance( + List( + version(3, 0L, positional), // latest — important regardless of content + version(2, TimeUnit.SECONDS.toMillis(1), meaningful), // within the aggregate window + version(1, TimeUnit.DAYS.toMillis(30), meaningful) // outside it, judged on content + ) + ) + + encoded.map(_.vId) shouldBe List(3, 2, 1) + encoded.map(_.importance) shouldBe List(true, false, true) + } } diff --git a/amber/src/test/scala/org/apache/texera/web/service/WorkflowEmailNotifierSpec.scala b/amber/src/test/scala/org/apache/texera/web/service/WorkflowEmailNotifierSpec.scala index af21c7f687..aecc3dc3fb 100644 --- a/amber/src/test/scala/org/apache/texera/web/service/WorkflowEmailNotifierSpec.scala +++ b/amber/src/test/scala/org/apache/texera/web/service/WorkflowEmailNotifierSpec.scala @@ -25,6 +25,7 @@ import org.apache.texera.dao.MockTexeraDB import org.apache.texera.dao.jooq.generated.Tables.WORKFLOW import org.apache.texera.dao.jooq.generated.tables.daos.WorkflowDao import org.apache.texera.dao.jooq.generated.tables.pojos.Workflow +import org.apache.texera.web.resource.EmailMessage import org.scalatest.flatspec.AnyFlatSpec import org.scalatest.{BeforeAndAfterAll, PrivateMethodTester} @@ -64,6 +65,7 @@ class WorkflowEmailNotifierSpec private val testWorkflowName: String = "notifier_wf_" + UUID.randomUUID().toString.substring(0, 8) private val userEmail = "[email protected]" + private val createEmailMessage = PrivateMethod[EmailMessage](Symbol("createEmailMessage")) private val createDashboardUrl = PrivateMethod[String](Symbol("createDashboardUrl")) private val createEmailSubject = PrivateMethod[String](Symbol("createEmailSubject")) private val createEmailContent = PrivateMethod[String](Symbol("createEmailContent")) @@ -216,4 +218,35 @@ class WorkflowEmailNotifierSpec } assert(ex.getMessage.contains(unseededWid.toString)) } + + // ─── createEmailMessage ──────────────────────────────────────────────────── + + "createEmailMessage" should "address the recipient and carry the subject and content for the state" in { + val notifier = notifierFor("http://texera.example.com:8080/dashboard";) + val message = notifier invokePrivate createEmailMessage(KILLED) + + assert(message.receiver == userEmail) + // the assembled subject must match what the dedicated builder produces for the same state + assert(message.subject == (notifier invokePrivate createEmailSubject(KILLED))) + assert(message.content.contains(testWorkflowName)) + assert(message.content.contains(testWid.toString)) + assert(message.content.contains(KILLED.name)) + } + + // ─── sendStatusEmail ─────────────────────────────────────────────────────── + + // Only the invalid-recipient arm is driven here: it returns before reaching + // GmailResource.sendEmail, so no SMTP call is made. The valid-address arm would dispatch + // for real, so it belongs to the integration tier. + "sendStatusEmail" should "return without dispatching when the recipient address is invalid" in { + val notifier = + new WorkflowEmailNotifier( + testWid.toLong, + "not-an-email", + new URI("http://texera.example.com";) + ) + + notifier.sendStatusEmail(COMPLETED) // must not throw and must not reach Gmail + assert(!(notifier invokePrivate isValidEmail("not-an-email"))) + } }
