-
Notifications
You must be signed in to change notification settings - Fork 154
Feature request: Change MetricsInterface.singleMetric() to have return type MetricsInterface #3132
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
Comments
Hi @mikebroberts, thank you for opening the issue. Before getting into the details of the request, have you seen this section of the docs? By setting the Would this help? Most the customers who use Metrics use this method in their tests |
@dreamorosi - yup, I saw that Andrea, thanks. My preference is not to use environment level configuration for unit tests since I might run them from a number of contexts - command line, IDE, etc. And so if I can do everything within the code of the unit test then that to me is better. That way whenever I run the test I get the behavior I'm after. I do fully admit this is a very small edge case, however I felt that (a) the change is small - if I'm right then just one line, and (b) it means that it's a slightly cleaner design anyway since there's not a circular dependency between the interface and the implementation. |
Yea, I'm not against the change in itself, we've done the same for Logger in another issue if I recall correctly. Regarding the env variables in tests, you can still do it programmatically via setup files or at any point before importing your code, but I'm sure you know this and it's besides the point. Ultimately it's about preference and I respect that. Before I decide to put this in the backlog, allow me to take a deeper look at the change. At surface level I'm inclined to agree with your initial assessment, so I don't see issues but I want to be sure it's not causing issues elsewhere. It's already weekend where I'm based, so I'm going to get back to you here on Monday. |
Thanks @dreamorosi - no rush on my side. :) I was just cleaning up a few things on my side and noticed this. |
I have started looking into this and I see no issues with moving forward. I'll open a PR to change this shortly and the change will likely land in the next release (ETA ~next week). |
This issue is now closed. Please be mindful that future comments are hard for our team to see. If you need more assistance, please either tag a team member or open a new issue that references this one. If you wish to keep having a conversation with other community members under this issue feel free to do so. |
This is now released under v2.9.0 version! |
Use case
Metrics doesn't (currently) have a good way to silence all output during unit tests (it requires configuration changes outside the scope of the unit test code.) To work around this I was going to use a fake implementation of the
MetricsInterface
interface, and pass that to actual code that generates metrics, instead of passing a reference to the concreteMetrics
class. At unit test time I can instead pass a fake implementation of this interface that just silently swallows all requests. The problem with this workaround is that theMetricsInterface.singleMetric()
method has to return a concrete implementation ofMetrics
.Solution/User Experience
Change
MetricsInterface.singleMetric()
to returnMetricsInterface
. I don't think (?) this should break anything, and will allow me to replace the concrete implementation at unit test time.Alternative solutions
Acknowledgment
Future readers
Please react with 👍 and your use case to help us understand customer demand.
The text was updated successfully, but these errors were encountered: