Skip to content

Chore: Replace assertEquals with assertSame#1

Merged
lohanidamodar merged 1 commit into
mainfrom
chore-assert-same
Nov 26, 2025
Merged

Chore: Replace assertEquals with assertSame#1
lohanidamodar merged 1 commit into
mainfrom
chore-assert-same

Conversation

@Meldiron

@Meldiron Meldiron commented Nov 26, 2025

Copy link
Copy Markdown
Contributor

Summary by CodeRabbit

  • Tests
    • Strengthened test assertions to enforce stricter type and value equality checks, improving validation of async retry behavior and preventing false positives in test scenarios.

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

@coderabbitai

coderabbitai Bot commented Nov 26, 2025

Copy link
Copy Markdown

Walkthrough

The pull request modifies test assertions across two files to enforce stricter type and value comparison. In README.md and tests/AsyncTest.php, loose equality assertions (assertEquals) are replaced with strict equality assertions (assertSame). The changes affect a single test probe in README.md and three counter assertions across two test methods in tests/AsyncTest.php. The underlying test logic and control flow remain unchanged.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~5 minutes

Rationale: All changes follow a homogeneous refactoring pattern (assertEquals → assertSame), affecting only two files with repetitive, identical modifications. No new logic is introduced, and the changes are consistent cosmetic upgrades to assertion strictness.

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'Chore: Replace assertEquals with assertSame' accurately reflects the main changes in the pull request, which involve replacing assertEquals with assertSame assertions across test files.
✨ 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 chore-assert-same

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.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 3212608 and 4c8e344.

📒 Files selected for processing (2)
  • README.md (1 hunks)
  • tests/AsyncTest.php (2 hunks)
🔇 Additional comments (2)
tests/AsyncTest.php (1)

85-88: LGTM! Stricter type checking is appropriate here.

The change from assertEquals to assertSame correctly enforces strict equality (type + value). Since $counter is always an integer, this is the more appropriate assertion method and follows PHPUnit best practices.

README.md (1)

52-52: LGTM! Documentation updated to demonstrate best practices.

The change from assertEquals to assertSame in the documentation example is consistent with the PR's goal and demonstrates the recommended stricter assertion method for users.

Comment thread tests/AsyncTest.php

// Counter should be exactly 2, not more
$this->assertEquals(2, $counter);
$this->assertSame(2, $counter);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Unreachable assertion after expectException.

This assertion will never execute because the Critical exception thrown at line 47 is caught by PHPUnit (via expectException at line 42), ending the test immediately. The counter verification intended here never occurs.

Consider removing this line or restructuring the test to verify the counter value in a way that's actually reachable.

Note: This is a pre-existing issue not introduced by this PR, but worth addressing while modifying this line.

🤖 Prompt for AI Agents
In tests/AsyncTest.php around line 53, the assertion `$this->assertSame(2,
$counter);` is unreachable because PHPUnit's expectException causes the test to
end when the Critical exception is thrown; either remove this unreachable
assertion or restructure the test so the counter is verified: either (a) move
the counter assertion to before the point that triggers the exception, or (b)
remove expectException and wrap the code that throws in a try/catch, asserting
the counter in the catch (and rethrow or assert the exception) so the counter
check runs.

@lohanidamodar lohanidamodar merged commit ca8bf46 into main Nov 26, 2025
4 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