Skip to content

test(amber): add unit test coverage for AddressInfo#4916

Merged
aglinxinyuan merged 4 commits into
apache:mainfrom
aglinxinyuan:test-address-info-spec
May 4, 2026
Merged

test(amber): add unit test coverage for AddressInfo#4916
aglinxinyuan merged 4 commits into
apache:mainfrom
aglinxinyuan:test-address-info-spec

Conversation

@aglinxinyuan
Copy link
Copy Markdown
Contributor

@aglinxinyuan aglinxinyuan commented May 4, 2026

What changes were proposed in this PR?

Adds AddressInfoSpec covering AddressInfo (amber/src/main/scala/org/apache/texera/amber/engine/architecture/deploysemantics/AddressInfo.scala), the case class the cluster scheduler uses to describe worker / controller node addresses. It has no test coverage today.

The new spec pins:

  • Both fields (allAddresses, controllerAddress) round-trip from the case-class constructor.
  • The order of allAddresses is preserved (the cluster scheduler picks workers based on this order, so any reorder is observable).
  • An empty allAddresses array is accepted (controller-only configurations).
  • The controller address may also appear in allAddresses (collocated mode).
  • copy() allows one field to change while preserving the other.
  • Case-class equality with an Array field is reference-based, not element-wise. This is locked down explicitly so a future change to (say) Seq doesn't silently flip the equality semantics for callers.

No production code changed; this is test-only.

Any related issues, documentation, discussions?

Closes #4915

How was this PR tested?

Added 6 new unit tests in AddressInfoSpec. Verified locally:

sbt 'WorkflowExecutionService/Test/testOnly org.apache.texera.amber.engine.architecture.deploysemantics.AddressInfoSpec'
# → Tests: succeeded 6, failed 0

sbt 'WorkflowExecutionService/Test/scalafmtCheck'
# → clean

Was this PR authored or co-authored using generative AI tooling?

Generated-by: Claude Code

Pin the contract of `AddressInfo`:
- Both fields (`allAddresses`, `controllerAddress`) round-trip from
  the case-class constructor.
- Order of `allAddresses` is preserved (the cluster scheduler picks
  workers based on this order).
- Empty `allAddresses` is accepted (controller-only configurations).
- The controller may also appear in `allAddresses` (collocated mode).
- `copy()` lets a single field change while preserving the other.
- Case-class equality on the Array field is reference-based, not
  element-wise — locked down explicitly so a switch to (say) Seq
  doesn't silently flip the equality semantics for callers.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds missing unit test coverage for AddressInfo in Amber’s deploy-semantics layer, pinning expected case-class behavior that the cluster scheduler and callers may rely on (including Array equality semantics).

Changes:

  • Introduces AddressInfoSpec with 6 unit tests covering constructor round-trip, allAddresses ordering, empty/collocated configurations, copy() behavior, and Array reference-equality semantics.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

…nce semantics

Address Copilot feedback on apache#4916: the test description previously read
"fields match by value", but the assertions specifically pin Array
*reference* equality (case-class equality with an `Array` field is
reference-based, not element-wise). Rename to "use Array reference
equality (not element-wise) for the allAddresses field" and rename the
local bindings (sameRef / sameRefAgain / differentRef) to match.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented May 4, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 43.32%. Comparing base (1872ca0) to head (95572eb).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff              @@
##               main    #4916      +/-   ##
============================================
+ Coverage     42.17%   43.32%   +1.14%     
+ Complexity     2181     2179       -2     
============================================
  Files           980      915      -65     
  Lines         36298    32189    -4109     
  Branches       3783     3256     -527     
============================================
- Hits          15308    13945    -1363     
+ Misses        20065    17392    -2673     
+ Partials        925      852      -73     
Flag Coverage Δ
amber 43.12% <ø> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@aglinxinyuan aglinxinyuan enabled auto-merge (squash) May 4, 2026 07:41
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@aglinxinyuan aglinxinyuan merged commit aa89662 into apache:main May 4, 2026
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

add unit test coverage for AddressInfo

4 participants