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

[sdk/logs] Replace mocks with real instances where possible #4071

Merged
merged 2 commits into from
Jul 23, 2024

Conversation

pmcollins
Copy link
Member

This change replaces some mocks with real instances in log testing. Motivated by looking at tests in this PR which uses mocks when they aren't strictly necessary. Would like to suggest that that change's tests use real types, but still be consistent with the other tests.

@pmcollins pmcollins requested a review from a team July 22, 2024 21:57
@pmcollins pmcollins marked this pull request as draft July 22, 2024 21:57
@pmcollins pmcollins added the Skip Changelog PRs that do not require a CHANGELOG.md entry label Jul 23, 2024
@pmcollins pmcollins marked this pull request as ready for review July 23, 2024 14:39
@lzchen lzchen merged commit be02f98 into open-telemetry:main Jul 23, 2024
284 checks passed
@ocelotl
Copy link
Contributor

ocelotl commented Jul 23, 2024

Actually, I think we want to use mocks as much as possible. When we test a particular code, ideally, we want to mock everything else. Mocks are cheap and this way we don't waste resources instantiating any real objects that could be mocked instead.

@pmcollins pmcollins deleted the replace-mocks branch July 23, 2024 19:55
@pmcollins
Copy link
Member Author

Actually, I think we want to use mocks as much as possible. When we test a particular code, ideally, we want to mock everything else. Mocks are cheap and this way we don't waste resources instantiating any real objects that could be mocked instead.

Happy to defer to the preferences of the folks in this SIG, and this is perhaps a larger topic than we might want to address here, but I think using real types in tests has lots of benefits over using mocks, mostly because real types more accurately reflect how the system will behave. This helps with verifying correctness at dev time, but it also helps with catching regressions later, and with test code demonstrating how to use an API. It's true that mocks can be faster than real types, especially types that perform I/O, but making I/O swappable for testing and using fakes during testing solves this problem (IMO it also nudges you to do other beneficial things like using dependency inversion and grouping related I/O code).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Skip Changelog PRs that do not require a CHANGELOG.md entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants