test(amber): cover the mail resource's address guard and admin fan-out - #7631
test(amber): cover the mail resource's address guard and admin fan-out#7631aglinxinyuan wants to merge 2 commits into
Conversation
GmailResource sat at 31.2% of 77 lines in isolation. Despite the name it is a javax.mail/SMTP resource, not a Google-OAuth one: there is no GoogleCredential, no token exchange, no clientId, and the only credential is an SMTP app password consumed inside an Authenticator callback that never fires unless a socket opens. So the parts worth testing were never credential-gated. Adds 4 tests, taking it to 61.0%. The recipient-format guard is pinned clause by clause, including the null short-circuit and the TLD floor, with a companion test asserting a well-formed address fails later in delivery so the malformed list cannot pass under a reject-everything regex. The admin fan-out is driven with two admins and a REGULAR decoy, so narrowing the role filter or notifying only the first admin both fail. No socket is opened. Two unreachable catch arms and the applicant acknowledgement are recorded as unpinnable rather than cemented, as is the fact that only the notification's recipient is pinned and not its payload. No production file is touched.
Automated Reviewer SuggestionsBased on the
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7631 +/- ##
============================================
- Coverage 89.62% 89.31% -0.32%
+ Complexity 4395 4382 -13
============================================
Files 1177 1176 -1
Lines 46884 46845 -39
Branches 5239 5238 -1
============================================
- Hits 42022 41840 -182
- Misses 3109 3247 +138
- Partials 1753 1758 +5
*This pull request uses carry forward flags. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
| config | throughput | MB/s | latency | max Δ latest / 7d | |
|---|---|---|---|---|---|
| 🔴 | bs=10 sw=10 sl=64 | 352 | 0.215 | 27,636/39,253/39,253 us | 🔴 -15.0% / 🔴 +140.7% |
| 🟢 | bs=100 sw=10 sl=64 | 808 | 0.493 | 121,933/139,553/139,553 us | 🟢 -5.5% / 🔴 +29.0% |
| ⚪ | bs=1000 sw=10 sl=64 | 918 | 0.56 | 1,090,973/1,125,571/1,125,571 us | ⚪ within ±5% / 🔴 -10.6% |
Baseline details
Latest main eefd860 from same runner
| config | metric | PR | latest main | 7d avg | Δ latest | Δ 7d |
|---|---|---|---|---|---|---|
| bs=10 sw=10 sl=64 | throughput | 352 tuples/sec | 414 tuples/sec | 775.33 tuples/sec | -15.0% | -54.6% |
| bs=10 sw=10 sl=64 | MB/s | 0.215 MB/s | 0.253 MB/s | 0.473 MB/s | -15.0% | -54.6% |
| bs=10 sw=10 sl=64 | p50 | 27,636 us | 25,267 us | 12,743 us | +9.4% | +116.9% |
| bs=10 sw=10 sl=64 | p95 | 39,253 us | 34,356 us | 16,310 us | +14.3% | +140.7% |
| bs=10 sw=10 sl=64 | p99 | 39,253 us | 34,356 us | 18,926 us | +14.3% | +107.4% |
| bs=100 sw=10 sl=64 | throughput | 808 tuples/sec | 812 tuples/sec | 1,001 tuples/sec | -0.5% | -19.3% |
| bs=100 sw=10 sl=64 | MB/s | 0.493 MB/s | 0.495 MB/s | 0.611 MB/s | -0.4% | -19.3% |
| bs=100 sw=10 sl=64 | p50 | 121,933 us | 121,865 us | 101,399 us | +0.1% | +20.3% |
| bs=100 sw=10 sl=64 | p95 | 139,553 us | 147,717 us | 108,206 us | -5.5% | +29.0% |
| bs=100 sw=10 sl=64 | p99 | 139,553 us | 147,717 us | 118,195 us | -5.5% | +18.1% |
| bs=1000 sw=10 sl=64 | throughput | 918 tuples/sec | 929 tuples/sec | 1,026 tuples/sec | -1.2% | -10.5% |
| bs=1000 sw=10 sl=64 | MB/s | 0.56 MB/s | 0.567 MB/s | 0.626 MB/s | -1.2% | -10.6% |
| bs=1000 sw=10 sl=64 | p50 | 1,090,973 us | 1,072,508 us | 996,304 us | +1.7% | +9.5% |
| bs=1000 sw=10 sl=64 | p95 | 1,125,571 us | 1,136,698 us | 1,042,531 us | -1.0% | +8.0% |
| bs=1000 sw=10 sl=64 | p99 | 1,125,571 us | 1,136,698 us | 1,074,934 us | -1.0% | +4.7% |
Raw CSV
config_idx,batch_size,schema_width,string_len,num_batches,total_ms,total_tuples,total_bytes,tuples_per_sec,mb_per_sec,lat_p50_us,lat_p95_us,lat_p99_us
0,10,10,64,20,568.98,200,128000,352,0.215,27635.93,39253.21,39253.21
1,100,10,64,20,2473.77,2000,1280000,808,0.493,121932.73,139553.38,139553.38
2,1000,10,64,20,21779.65,20000,12800000,918,0.560,1090973.49,1125571.17,1125571.17There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
Suppressed comments (3)
amber/src/test/scala/org/apache/texera/web/resource/GmailResourceSpec.scala:183
- Overrides of BeforeAndAfterEach hooks should call super.beforeEach()/super.afterEach() so other mixins (present or future) get their per-test setup/teardown; also wrap cleanup in finally so it runs even if the test fails.
override protected def beforeEach(): Unit = {
userDao = new UserDao(getDSLContext.configuration())
cleanup()
}
override protected def afterEach(): Unit = cleanup()
amber/src/test/scala/org/apache/texera/web/resource/GmailResourceSpec.scala:176
- Overrides of BeforeAndAfterAll hooks should call super.beforeAll()/super.afterAll() to keep ScalaTest’s lifecycle stackable (and ensure any future mixed-in traits still run their setup/teardown).
This issue also appears on line 177 of the same file.
override protected def beforeAll(): Unit = initializeDBAndReplaceDSLContext()
override protected def afterAll(): Unit = shutdownDB()
amber/src/test/scala/org/apache/texera/web/resource/GmailResourceSpec.scala:170
- The comment says “suites run sequentially”, but amber explicitly does not set
Test / parallelExecution := false(see amber/src/test/scala/org/apache/texera/web/ComputingUnitMasterSpec.scala:131-132) while MockTexeraDB’s SqlServer singleton design assumes sequential suites. This suite now mixes in MockTexeraDB, so it can become flaky if suites overlap in the same JVM; please either enforce serial execution for WorkflowExecutionService tests (e.g., setTest / parallelExecution := falseand/orTest / fork := truefor the project) or avoid relying on the shared SqlServer singleton here.
// Random suffix keeps this suite's rows from colliding with data other suites may
// have left in the shared (singleton) embedded DB; suites run sequentially.
private val runId = UUID.randomUUID().toString.substring(0, 8)
|
The red amber jobs here are not from this PR —
Two PRs raced: #7563 renamed the field, #7580 added assertions against the old name, and each was green against its own base. Verified by stashing a fix on an otherwise-clean Fix is up as #7633 (two-line rename). Once that lands I will rebase this and CI should go green. |
|
Correcting my own comment above: I said this "takes them down on every open PR", which overstates it. Accurately — any PR whose amber jobs run after the breakage landed fails on it (this PR is one), while PRs whose amber jobs ran before it still show green and would fail on re-run, and frontend-labelled PRs skip the amber stack entirely. So it is every amber run from now until #7633 lands, not every open PR as of today. |
…ache#7633) ### What changes were proposed in this PR? **`main` does not compile.** Any PR whose `build / amber` or `build / amber-integration` jobs run after this landed fails on it regardless of what the PR itself touches — apache#7631 is an example, where the only change is four tests in an unrelated module's spec. Scoping that honestly: PRs whose amber jobs ran *before* the breakage still show green and would fail on re-run, and frontend-labelled PRs skip the amber stack, so this is "every amber run from now until it is fixed" rather than "every open PR is red today". `ComputingUnitManagingResourceSpec` asserts on `DashboardWorkflowComputingUnit.ownerGoogleAvatar`, but the field is named `ownerAvatar`, so `ComputingUnitManagingService / Test` fails with two "value ownerGoogleAvatar is not a member" errors. Two PRs raced to produce it: apache#7563 renamed the field to `ownerAvatar`, while apache#7580 added assertions written against the old name. Each was green against its own base, and the combination is what breaks — the kind of thing per-PR CI cannot see when two PRs touch different files. This renames the two accessor calls. Nothing else changes. ### How was this PR tested? Confirmed the breakage is real and that this is the whole of it, by stashing the change and re-running on otherwise-clean `main`: ``` sbt "ComputingUnitManagingService/Test/compile" ``` | | Result | |---|---| | unpatched `main` | exit 1, exactly 2 × `value ownerGoogleAvatar is not a member` | | with this change | exit 0, compiles clean | Then the spec itself: ``` sbt "ComputingUnitManagingService/testOnly org.apache.texera.service.resource.ComputingUnitManagingResourceSpec" ``` ``` [info] Total number of tests run: 31 [info] Tests: succeeded 31, failed 0, canceled 0, ignored 0, pending 0 ``` All 31 pass, so the assertions were correct about the value and only the accessor name was stale. `Test/scalafmtCheck` and `Test/scalafix --check` both pass. ### Any related issues, documentation, discussions? Closes apache#7632 ### Was this PR authored or co-authored using generative AI tooling? Generated-by: Claude Code (Opus 5)
What changes were proposed in this PR?
GmailResourcesat at 31.2% of 77 lines in isolation. Despite the name this is a javax.mail/SMTP resource, not a Google-OAuth one — noGoogleCredential, no token exchange, noclientId, and the only credential is an SMTP app password consumed inside anAuthenticatorcallback that never fires without a socket. Nothing worth testing here was credential-gated.Adds 4 tests to the existing spec, taking it to 61.0% (Codecov-style; 85.7% JaCoCo line). No socket is opened — the existing
requireNoRealGmailSender()guard is kept and called from every new test that reachessendEmail.Covered: the recipient-format guard clause by clause, and the admin fan-out driven through
MockTexeraDB.Verification
9 mutations applied and reverted, production diff empty each time. The interesting ones:
ADMIN→REGULARwhile (hasNext)→ notify only the first adminwhile→do/while(assume at least one admin){2,}→{1,}"^$"(reject everything)One mutation exposed a coupling in my own fixture rather than a defect: loosening the domain regex made the seeded
…@localhostadmin addresses deliverable, which false-reddened the fan-out test. The fixture now uses addresses with no@at all, so the fan-out is no longer coupled to the regex's domain rules, and every mutation was re-run afterwards.Deliberately not included
userRegistrationNotification(...)construction with the incoming request leaves the suite green:sendEmailis on the companion object so nothing can intercept what it received, and the only offline observable is which address it rejected. The builder's contract — including itstoAdminbranch — is already pinned byEmailTemplateSpec, which is where it belongs, so this is recorded rather than duplicated.getSenderEmail. The analyst's plan listed this as a win; I disagree and dropped it. BothUserSystemConfig.gmailandsmtpPassworddefault to""in an unconfigured JVM, so the only available assertion cannot distinguish this endpoint from one that leaks the SMTP password. A test there would be vacuous.catcharms innotifyUnauthorizedUserare unreachable —sendEmailwraps everything in aTryand returns aLeft, never throwing. A comment says so and tells the next reader to delete them rather than reach them.createMimeMessage's tail, and thewithDomainSomearm — the first has no observable effect offline, the second armsTransport.sendon the next statement, and the third needsTest/envVarsinbuild.sbt, which would perturb every other spec.No production file is touched.
Any related issues, documentation, discussions?
Closes #7630
How was this PR tested?
4 new on top of the existing 4.
Test/scalafmtCheckandTest/scalafix --checkboth pass.Was this PR authored or co-authored using generative AI tooling?
Generated-by: Claude Code (Opus 5)