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

Expose TestObservationRegistry as an AssertJ AssertProvider #5551

Merged
merged 1 commit into from
Oct 7, 2024

Conversation

filiphr
Copy link
Contributor

@filiphr filiphr commented Oct 5, 2024

This is just an idea which is inspired by the way the Spring Boot ApplicationContextAssertProvider works. Some more information from the Spring Boot Testing your Auto-configuration.

The main idea is basically for people not to need to do TestObservationRegistryAssert.assertThat, but instead use (the most likely already exposed) Assertions.assertThat.

I tried to look into the issues and PRs if something like this was proposed before, but I could not find anything and I thought to share an idea as a PR. I fully understand that perhaps the idea of the TestObservationRegistry is not to depend on AssertJ and / or you do not like the idea at all. In any case if you think that this is not useful for the project then please go ahead and reject the PR.

Copy link
Member

@shakuzen shakuzen left a comment

Choose a reason for hiding this comment

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

Thanks for the pull request. It sounds like a nice change to me. We already declare an API dependency on AssertJ in the micrometer-observation-test module (see here), so I don't think it would be a big deal for the TestObservationRegistry to implement API from AssertJ. Let's see what the rest of the team thinks.

@shakuzen shakuzen added this to the 1.14.0-RC1 milestone Oct 7, 2024
@shakuzen
Copy link
Member

shakuzen commented Oct 7, 2024

I'm not sure why CircleCI did not run. Could you try updating the branch to trigger the CI to run again?

@filiphr filiphr force-pushed the assertj-assert-provider branch from 886fc05 to a622edf Compare October 7, 2024 06:06
@filiphr
Copy link
Contributor Author

filiphr commented Oct 7, 2024

Thanks @shakuzen for looking into this. I rebased this on top of main, but it seems like CircleCI didn't run again, it's the same problem as in my other PR.

Is there a reason why you are not using GitHub Actions? Would you be interested in a PR which adds builds for them?

@shakuzen
Copy link
Member

shakuzen commented Oct 7, 2024

Is there a reason why you are not using GitHub Actions?

The short answer is: CircleCI is what has historically been used, it's free, and we have very few problems with it. If it ain't broke, don't fix it, as they say. That said, this issue happens rarely when users have setup a CircleCI account before and so it is trying to build the PR on their CircleCI account as opposed to the Micrometer org account, but their account auth is no longer valid for CircleCI. I'm not certain that's what is happening here, but it's a possibility.

@filiphr filiphr force-pushed the assertj-assert-provider branch from a622edf to 66b7143 Compare October 7, 2024 07:11
@filiphr
Copy link
Contributor Author

filiphr commented Oct 7, 2024

You were right, I did have a CircleCI account setup from long time ago for some other Open Source projects I am working on. I've logged in there and now it looks like CircleCI is running. I'll adjust the other PR as well.

If it ain't broke, don't fix it, as they say.

100% agree with you. Was mostly curious if there was not some other reason in particular.

@marcingrzejszczak
Copy link
Contributor

I think this is a great idea!

@shakuzen shakuzen merged commit ceb213a into micrometer-metrics:main Oct 7, 2024
7 checks passed
@filiphr filiphr deleted the assertj-assert-provider branch October 7, 2024 08:41
bclozel pushed a commit to spring-projects/spring-framework that referenced this pull request Nov 21, 2024
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