Skip to content

Use MeterRegistry.isClosed() for tests #14975

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

Closed

Conversation

dreis2211
Copy link
Contributor

Hi,

while working on #14972 I noticed that in quite some tests in this package we have a method called spyOnDisposableBean that gets duplicated and seems rather complicated - at least for the purpose of just being able to verify that stop() is called on MeterRegistry instances.

My initial thought here was to extract this into some utility class to at least get rid of the duplication. The problem here was to find an appropriate place and name for the class. And of course, the complexity of the method remained.

So after giving this a bit more thought, I found a solution that doesn't need reflection and spying at all. We could simply check for MeterRegistry.isClosed(), which is slightly different to stop(), but still gets the job done if you ask me. The destroyMethod of the underlying bean seems to point to close() anyway, so we should be safe here.

Let me know what you think.
Cheers,
Christoph

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Oct 27, 2018
@dreis2211
Copy link
Contributor Author

Concourse build seems to have stopped simply - I can't see if the failure is related. Local tests run through.

@philwebb
Copy link
Member

Probably just a CI wobble so don't worry about it. I like the change a lot!

@philwebb philwebb added type: task A general task and removed status: waiting-for-triage An issue we've not yet triaged labels Oct 27, 2018
@philwebb philwebb added this to the 2.1.x milestone Oct 27, 2018
@wilkinsona wilkinsona self-assigned this Oct 29, 2018
@wilkinsona wilkinsona modified the milestones: 2.1.x, 2.0.x Oct 29, 2018
wilkinsona added a commit that referenced this pull request Oct 29, 2018
* gh-14975:
  Use MeterRegistry.isClosed() for tests
@wilkinsona wilkinsona modified the milestones: 2.0.x, 2.0.7 Oct 29, 2018
@wilkinsona
Copy link
Member

wilkinsona commented Oct 29, 2018

Thanks very much, @dreis2211. I've merged the relevant parts of this into 2.0.x, adding in the parts that are specific to Boot 2.1/Micrometer 1.1 in the merge forwards to master.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: task A general task
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants