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

Test: Some tests overwrite the global logger for tests #4056

Closed
daipom opened this issue Feb 14, 2023 · 3 comments · Fixed by #4091
Closed

Test: Some tests overwrite the global logger for tests #4056

daipom opened this issue Feb 14, 2023 · 3 comments · Fixed by #4091
Assignees
Labels
bug Something isn't working

Comments

@daipom
Copy link
Contributor

daipom commented Feb 14, 2023

Describe the bug

Fluentd prepares a global logger with DummyLogDevice for tests.

fluentd/test/helper.rb

Lines 151 to 155 in bf7498d

dl_opts = {}
dl_opts[:log_level] = ServerEngine::DaemonLogger::WARN
logdev = Fluent::Test::DummyLogDevice.new
logger = ServerEngine::DaemonLogger.new(logdev, dl_opts)
$log ||= Fluent::Log.new(logger)

However, some tests overwrite this global logger.

  • logger.init(:supervisor, 0)
  • def create_debug_dummy_logger
    dl_opts = {}
    dl_opts[:log_level] = ServerEngine::DaemonLogger::DEBUG
    logdev = Fluent::Test::DummyLogDevice.new
    logger = ServerEngine::DaemonLogger.new(logdev, dl_opts)
    $log = Fluent::Log.new(logger)
    end
    def create_info_dummy_logger
    dl_opts = {}
    dl_opts[:log_level] = ServerEngine::DaemonLogger::INFO
    logdev = Fluent::Test::DummyLogDevice.new
    logger = ServerEngine::DaemonLogger.new(logdev, dl_opts)
    $log = Fluent::Log.new(logger)
    end
  • sv = Fluent::Supervisor.new(opts)
    log = sv.instance_variable_get(:@log)
    log.init(:standalone, 0)
    assert_equal Fluent::LogDeviceIO, $log.out.class

Is this as intended?
Is this affecting other tests depending on the test order?

I was trying to fix test_logger_initializer in the following PR, and I found this test may affect other tests.

I thought I needed to make sure the logger on the global was not changed, but when I looked into it, other tests are also overwriting the logger on the global, so I am not sure what is correct...

-> This can cause unintended behavior depending on the test order, so we should fix this.

To Reproduce

None

Expected behavior

None

Your Environment

None

Your Configuration

None

Your Error Log

None

Additional context

No response

@daipom
Copy link
Contributor Author

daipom commented Feb 14, 2023

I just understood.

def self.setup
ENV['SERVERENGINE_WORKER_ID'] = '0'
$log = dummy_logger

Plugin tests uses Fluentd::Test.setup() and this initializes $log.
This recovers some tests' overwriting, so the current tests work correctly.

However, this can cause problems depending on the test order, and we should fix this.

@daipom daipom closed this as completed Feb 14, 2023
@daipom daipom changed the title Test: Is the global logger for tests treated as intended? Test: Some tests overwrite the global logger for tests Feb 14, 2023
@daipom daipom reopened this Feb 14, 2023
@daipom daipom self-assigned this Feb 14, 2023
@daipom daipom added the bug Something isn't working label Feb 14, 2023
@daipom
Copy link
Contributor Author

daipom commented Feb 14, 2023

I reopened this since we have to fix this.

I think we should fix some tests, but we should also fix LoggerInitializer.

$log = Fluent::Log.new(logger, @opts)

I think LoggerInitializer::init() should not overwrite $log directly.

I wonder if there was a reason for this...

@daipom
Copy link
Contributor Author

daipom commented Mar 16, 2023

Possibly there are other areas where the global logger is overwritten.
If we find it, we should fix it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant