Skip to content

Refactor: assertions#64

Merged
Meldiron merged 1 commit intomainfrom
refactor-assertions
Nov 26, 2025
Merged

Refactor: assertions#64
Meldiron merged 1 commit intomainfrom
refactor-assertions

Conversation

@lohanidamodar
Copy link
Contributor

@lohanidamodar lohanidamodar commented Nov 26, 2025

  • replace assertEquals with assertSame

Summary by CodeRabbit

Release Notes

  • Tests
    • Enhanced internal test validation to ensure more precise verification of system behavior and output consistency.

Note: This release contains internal quality improvements with no user-facing changes.

✏️ Tip: You can customize this high-level summary in your review settings.

- replace assertEquals with assertSame
@coderabbitai
Copy link

coderabbitai bot commented Nov 26, 2025

Walkthrough

This pull request refactors test assertions in tests/Orchestration/Base.php by replacing loose equality checks with strict identity checks. Specifically, assertEquals invocations are replaced with assertSame for scalar values (booleans, integers, strings) and arrays. The test flow and control logic remain unchanged; only the assertion verification method is refined to enforce type-safe exact comparisons.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

  • Verify that all assertEquals to assertSame conversions are semantically correct and won't introduce unexpected test failures due to type strictness
  • Check array/list comparisons with assertSame to ensure order and type sensitivity align with test expectations
  • Spot-check several converted assertions across different test scenarios to confirm no test semantics were inadvertently altered

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'Refactor: assertions' is related to the main change, which is replacing assertEquals with assertSame throughout the test file. However, it is overly broad and vague—it doesn't specify what kind of refactoring or which assertions were changed, making it less informative than it could be.
Docstring Coverage ✅ Passed Docstring coverage is 81.25% which is sufficient. The required threshold is 80.00%.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch refactor-assertions

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (1)
tests/Orchestration/Base.php (1)

652-692: Usage stats assertions benefit from assertSame, with one duplicate check

  • Making the container ID length, container names, and filtered IDs use assertSame is appropriate and encodes precise expectations on types and values.
  • Lines 686–687 assert the same expression twice ($statsName2[0]->getContainerName() === $stats2[0]->getContainerName()), which is redundant and can be safely reduced to a single assertion when convenient.

Also applies to: 699-711

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e8d8d04 and ee71c8d.

📒 Files selected for processing (1)
  • tests/Orchestration/Base.php (20 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
  • GitHub Check: Tests 8.1
  • GitHub Check: Tests 8.2
  • GitHub Check: Tests 8.3
  • GitHub Check: Tests nightly
🔇 Additional comments (12)
tests/Orchestration/Base.php (12)

40-51: Stricter boolean checks in testPullImage look good

Using assertSame(true|false, $response) tightens these checks and is appropriate if pull() is guaranteed to return real booleans, not 0/1 or stringy equivalents. Just rerun the suite to confirm no unexpected type issues surface.


108-140: testCreateContainer assertions are safely tightened

The switch to assertSame(true, $response) for remove() and to assertSame(1, $occurances) for the restart log count preserves intent while enforcing correct types (bool and int). No behavioral concern as long as remove() returns a boolean.


192-210: Network creation/listing now type‑strict on booleans

assertSame(true, $response) and assertSame(true, $foundNetwork) correctly encode the expectation that these are true booleans rather than truthy values. This is a good tightening of the contract with createNetwork() and listNetworks().


247-268: Boolean assertions for network connect/disconnect/remove are consistent

The new assertSame(true, $response) calls across networkConnect, networkDisconnect, and removeNetwork consistently assert real booleans. This matches the rest of the suite’s stricter style.


336-357: Stricter output and length checks in testExecContainer

Using assertSame for the exact output string and for $length vs strlen($output) is appropriate and guards against accidental type juggling. This is a solid tightening with no downside here.


378-379: Exact volume content comparison is a good fit for assertSame

The long literal string vs $output is an ideal case for assertSame and will catch any subtle encoding or type issues.


448-469: Timeout execution assertions are correctly made strict

Enforcing assertSame(true, $response) and exact string equality on $output aligns with the intended contract of execute() returning a boolean and the command producing a precise payload.


486-487: Container listing now enforces a real boolean flag

assertSame(true, $foundContainer) is fine here and clearly encodes that $foundContainer must be a boolean, not just truthy.


494-497: Filter list assertion is correctly made strict on ID

assertSame(self::$containerID, $response[0]->getId()) is appropriate as long as IDs are consistently typed (typically strings). This will catch any accidental int/string mismatches.


507-513: Strict booleans for container removal are consistent with the rest of the suite

Both remove('TestContainer', true) and remove('TestContainerTimeout', true) now require a real true. This is in line with the PR goal and should be safe if the adapter returns booleans.


527-547: Array comparisons in testParseCLICommand are a good use of assertSame

Using assertSame for the parsed command arrays ensures both ordering and element types match exactly, which makes sense for CLI argument parsing.


589-592: Strict zero‑count check in testRunRemove is appropriate

assertSame(0, count($statusResponse)) is a simple and correct tightening; no functional change beyond preventing 0 == '0'‑style leniency.

@Meldiron Meldiron merged commit 0bc15e7 into main Nov 26, 2025
7 checks passed
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