Skip to content

Conversation

@lhotari
Copy link
Member

@lhotari lhotari commented Oct 7, 2024

> [!WARNING]
> There is something going on w/ the update that is causing the tests to block seemingly indefinitely.
> Let's not merge until I get to the bottom of it. Digging into it now.

@lhotari lhotari self-assigned this Oct 7, 2024
Copy link
Contributor

@onobc onobc left a comment

Choose a reason for hiding this comment

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

Changes LGTM. Once we get a green CI then 💚

@onobc onobc added this to the 0.5.8 milestone Oct 16, 2024
@onobc onobc requested review from cbornet and onobc October 16, 2024 02:03
The cause of the test hang was that the test was incorrectly setting up
the spy on the type message builder impl. In previous Pulsar version of
TypedMessageBuilderImpl, the fact that the method sendAsync was being
called at mock setup time was not causing an issue. However, in the
latest impl it did not like that and was throwing things off.

Spied objects should always use the `doReturn|Answer|Throw()` family
as described in https://javadoc.io/doc/org.mockito/mockito-core/latest/org/mockito/Mockito.html#important-gotcha-on-spying-real-objects--heading
@onobc
Copy link
Contributor

onobc commented Oct 16, 2024

@lhotari the cause for hanging tests is described well in commit 3. PTAL.

Also, I searched through all usages of spy( and made sure we are using the willThrow|Return|Answer family for them.

Copy link
Contributor

@onobc onobc left a comment

Choose a reason for hiding this comment

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

CI is 💚 ... and the changes LGTM.

@lhotari
Copy link
Member Author

lhotari commented Oct 16, 2024

@lhotari the cause for hanging tests is described well in commit 3. PTAL.

Also, I searched through all usages of spy( and made sure we are using the willThrow|Return|Answer family for them.

Thanks for fixing this @onobc !

@lhotari lhotari merged commit a83a5be into apache:main Oct 16, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants