(FACT-3468) Don't mutate global logger state #2722
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
joshcooper
force-pushed
the
logging
branch
2 times, most recently
from
7390fe8
to
cd3d660
Compare
joshcooper
changed the title
…method
Previously, some resolvers eagerly created a Log, while others relied on the
BaseResolver to lazily create it.
Now eagerly create the Log in the BaseResolver's init_resolver method, which all
derived resolvers already call. This preserves the behavior where the derived class'
name is printed in the log message:
DEBUG Facter::Resolvers::LsbRelease - Command lsb_release ...
This is because `Log.new(self)` refers to the derived resolver class, e.g.
Facter::Resolvers::LsbRelease, not the BaseResolver.
The base_resolver_spec tests were modified because they were testing that
Facter::Log.new was called a certain way. Instead test the behavior that
`BaseResolver.log` method returns an instance of Facter::Log and calling the
method multiple times returns the same object.
Previously, many tests set an instance_spy logger on their resolver's `log` class variable. But only some of those tests cleared the instance_spy at the end. Therefore, the spy could leak from one test to another, see where one case was fixed in 06b1d5b. To prevent that from happening again, just let the resolver use its log. Since we're not using instance spies, we have to set the expectation on the log before calling the method that causes the log to be written to. One motivation for installing a spy is to prevent messages from being written to the terminal. But this accomplishes the same thing but installing a global Logger in spec_helper that just writes to a StringIO in memory.
Previously if an aggregate fact was evaluated twice it would attempt to log a warning but its `log` method was never defined, leading to NoMethodError. It appears this was broken in ee582c5 when the Legacy logger was removed. This commit adds a private `log` method to lazily create the log as is done elsewhere and adds a test. Since string literals are frozen, we need to dup prior to modifying it.
Previously, tests were clearing out Facter's global logger after each test. Now just set expectations on the logger. Since we're no longer using an instance_spy, the expectation needs to be set on the Logger before calling the subject.
Previously, the Collection's class variable was cleared after each test. Now resolve the logger and set expectations on the log. We have to use `send` because the `log` method is private.
Previously, we were overwriting the global logger with an instance spy. In some
cases we were not clearing it when we were done or we set it to a new instance
of Logger.new(STDOUT). In the first case, the instance spy could leak across to
other tests. In the second case, we've replaced the global logger with a
different instance instead of replacing it with the original instance.
All of this can be avoided by just getting the logger and setting expectations
on it. Since it's a "class instance variable" we have to use `:@@logger` syntax.
Since the global logger is not a spy, then we have to set the expectation before
calling the method that triggers the message to be sent such as:
expect(logger).to receive(:debug).with('...')
log.debug('debug_message')
Previously, we were stubbing Facter::Log.new to return our instance_spy. But we can just pass the instance_spy as an keyword argument to the `execute` method. Confusingly the `execute` method takes a `logger` keyword argument, but internally it creates a `Facter::Log` object, so the argument should really be called `log`, but that's part of the execution API and can't be changed.
If gethostname returned nil, then we called `empty?` on nil which raised a NoMethodError. Instead check for nil and don't attempt to resolve the hostname fact. Also cleanup some tests which were writing errors to the log unnecessarily.
joshcooper
force-pushed
the
logging
branch
2 times, most recently
from
85f476f
to
27e7fd5
Compare
cthorn42
approved these changes
May 31, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Facter::Logthe same way, in the BaseResolver.LoggerandFacter::Logclass variables instead of replacing them with instance spies.See commits for details
Fixes #2721