Skip to content

fix: prevent TestTransport from increasing phpunit's assertions count#104

Open
podoko wants to merge 1 commit into
zenstruck:1.xfrom
podoko:avoid-asserts-in-test-transprts
Open

fix: prevent TestTransport from increasing phpunit's assertions count#104
podoko wants to merge 1 commit into
zenstruck:1.xfrom
podoko:avoid-asserts-in-test-transprts

Conversation

@podoko

@podoko podoko commented May 31, 2026

Copy link
Copy Markdown

The transport is currently using asserts in its code that have two impacts :

  • hides risky tests that do not perform assertions from the developer (since the transport is doing assertions, the test is not flagged as risky)
  • prevent developers who actually want to use TestCase::expectNotToPerformAssertions in their tests since the transport might do assertions

The change aims to only do asserts on failed actions (for instance, failed serialization) in order for risky tests to be visible

 The transport was using asserts in its code that had two impacts :
- hiding risky tests from end users that do not perform assertions
- prevent users to actually using phpunit's `TestCase::expectNotToPerformAssertions` method

@nikophil nikophil left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hello, thank you for this PR, and sorry for the lack of review 😅

I'm afraid that this would break people's CIs, if they rely on the fact that messenger-test do actually perform assertions.

On the other hand, I think this is the way to go: the final user did not explicitly assert something, so the assertions counts should not be impacted.

WDYT if we introduce a new impacts_phpunit_assertions_count config (or a better name), which would default to true. And we would emit a deprecation if this value is true. The same way we are doing it for delay stamp support

In zenstruck/messenger-test v2 we wil deprecate this option, and always prevent the TestTransport to impact assertions count

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

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants