Skip to content

Test: add full payload JSON round-trip test in SystemStatusTests - #2221

Open
denizguney wants to merge 6 commits into
apple:mainfrom
denizguney:patch-4
Open

Test: add full payload JSON round-trip test in SystemStatusTests#2221
denizguney wants to merge 6 commits into
apple:mainfrom
denizguney:patch-4

Conversation

@denizguney

@denizguney denizguney commented Aug 30, 2026

Copy link
Copy Markdown

Adds a new unit test to verify that system status payloads with nested paths and resource counts correctly round-trip through JSON encoding and decoding.

Type of Change

  • Bug fix
  • New feature
  • Breaking change
  • Documentation update

Motivation and Context

Adds comprehensive JSON round-trip unit test coverage for SystemStatusTests to ensure correct encoding/decoding of nested paths and resource counts.

Testing

  • Tested locally
  • Added/updated tests
  • Added/updated docs

Adds a new unit test to verify that system status payloads with nested paths and resource counts correctly round-trip through JSON encoding and decoding.
let decoded = try JSONDecoder().decode(Application.StatusPayload.self, from: Data(json.utf8))

#expect(decoded.paths?.appRoot == "/app")
#expect(decoded.resources?.containersTotal == 10)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@denizguney This seems quite specific. Is it provable that these values will hold under all conditions?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

You're right, these values are currently specific to this sample test payload. Our main goal here is to isolate and verify the correct parsing and type safety of the StatusPayload structure. However, I can make the test more robust or document that this is strictly a deterministic validation for this specific mock fixture. Let me know if you'd prefer a more dynamic approach or if adjusting the fixture assertions works best!"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nah, I got it wrong. I thought I was looking at CLI tests. This test looks fine, please just fix the little whitespace nit and we can merge it.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Fixed the whitespace nit. Thanks for the review

let updated = Application.SystemStatus.withImageCount(resources, imageCount: nil)
#expect(updated?.images == nil)
}
@Test

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: add one line of white space

let decoded = try JSONDecoder().decode(Application.StatusPayload.self, from: Data(json.utf8))

#expect(decoded.paths?.appRoot == "/app")
#expect(decoded.resources?.containersTotal == 10)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nah, I got it wrong. I thought I was looking at CLI tests. This test looks fine, please just fix the little whitespace nit and we can merge it.

Add missing whitespace before the test method as requested in the review nit.

@denizguney denizguney left a comment

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Fixed the requested whitespace nit. Thanks again for the review

let decoded = try JSONDecoder().decode(Application.StatusPayload.self, from: Data(json.utf8))

#expect(decoded.paths?.appRoot == "/app")
#expect(decoded.resources?.containersTotal == 10)

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Fixed the whitespace nit. Thanks for the review

@jglogan

jglogan commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

@denizguney you'll need a make fmt run

@denizguney

Copy link
Copy Markdown
Author

"Fixed! Thanks for the review."

@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown

Code Coverage

Tier Line Coverage
Unit 24.96%
Integration 66.73%
Combined 76.27%

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants