-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Expose TestObservationRegistry as an AssertJ AssertProvider #5551
Conversation
There was a problem hiding this 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.
I'm not sure why CircleCI did not run. Could you try updating the branch to trigger the CI to run again? |
886fc05
to
a622edf
Compare
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? |
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. |
a622edf
to
66b7143
Compare
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.
100% agree with you. Was mostly curious if there was not some other reason in particular. |
I think this is a great idea! |
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.