Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use MockLogAppender.capturing #108114

Merged
merged 3 commits into from
May 2, 2024

Conversation

rjernst
Copy link
Member

@rjernst rjernst commented Apr 30, 2024

Many uses of MockLogAppender predate the addition of the helper method to create a Releasable for restoring logging, which allows the use of try-with-resource blocks. This commit converts current existing uses of MockLogAppender to use capturing.

relates #106425

Many uses of MockLogAppender predate the addition of the helper method
to create a Releasable for restoring logging, which allows the use of
try-with-resource blocks. This commit converts current existing uses of
MockLogAppender to use capturing.

relates elastic#106425
@rjernst rjernst added >test Issues or PRs that are addressing/adding tests :Core/Infra/Logging Log management and logging utilities labels Apr 30, 2024
@elasticsearchmachine elasticsearchmachine added v8.15.0 Team:Core/Infra Meta label for core/infra team labels Apr 30, 2024
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra (Team:Core/Infra)

@rjernst
Copy link
Member Author

rjernst commented Apr 30, 2024

Note that there are further cleanups here possible. For example, the creation of the MockLogAppender could happen as part of capturing, and the returned object could be the appender itself. However, for this PR I chose to make as mechanical a change as possible as a first step.

Copy link
Contributor

@ldematte ldematte left a comment

Choose a reason for hiding this comment

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

LGTM, only one minor nit: do we still need to call explicitly mockAppender.assertAllExpectationsMatched();? Since an identical check is done by the Releasable, maybe it's redundant and we can clean up them (can be as part of a follow-up, as you noted further cleanups are possible).

@rjernst
Copy link
Member Author

rjernst commented May 2, 2024

do we still need to call explicitly mockAppender.assertAllExpectationsMatched();

The check is not identical. It's actually checking that assert was called. Without the explicit assert call, a test will fail. Basically the releasable checks that an assert was done, but it doesn't presume that assertion should happen when releasing (though maybe it should, we can discuss for a followup).

@rjernst rjernst merged commit e63e8dd into elastic:main May 2, 2024
15 checks passed
@rjernst rjernst deleted the test/mock_appender_capturing branch May 2, 2024 13:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Core/Infra/Logging Log management and logging utilities Team:Core/Infra Meta label for core/infra team >test Issues or PRs that are addressing/adding tests v8.15.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants