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: don't use deprecated way of capturing logs #7

Merged
merged 1 commit into from
Apr 6, 2023

Conversation

daipom
Copy link
Contributor

@daipom daipom commented Feb 22, 2023

Background

The next td-agent will embed Ruby 3.2.

I was checking each embedded plugin and noticed that this plugin has no GitHub CI.

I added CI to my forked repository, but I found the tests are unstable on Windows and macOS.

So, I created this PR first.
Following this, I would like to create a PR to add GitHub CI.

About this fix

We should use Driver::logs instead of capture_log to fix unstable tests.

capture_log is defined by this plugin here.

class Test::Unit::TestCase
def capture_log(log)
if defined?(Fluent::Test::TestLogger) and log.is_a?(Fluent::Test::TestLogger) # v0.14
yield
log.out.logs.join("\n")
else
begin
tmp = log.out
log.out = StringIO.new
yield
return log.out.string
ensure
log.out = tmp
end
end
end
end

However, this is for older versions as well as the following, and this makes tests unstable.

Reproduce failing tests

Windows seems to have the highest probability of failure.

  • my forked repository's CI: https://github.com/daipom/fluent-plugin-flowcounter-simple/actions/runs/4239789791
  • On my Windows10 Home machine
    • Ruby 3.2
    • Each test sometimes succeeds and sometimes does not.
    $ bundle
    $ bundle exec rake
    C:/Ruby32-x64/bin/ruby.exe -w -I"lib;lib;test" C:/Ruby32-x64/lib/ruby/gems/3.2.0/gems/rake-13.0.6/lib/rake/rake_test_loader.rb "test/plugin/test_filter_flowcounter_simple.rb" "test/plugin/test_out_flowcounter_simple.rb"
    C:/Ruby32-x64/lib/ruby/gems/3.2.0/gems/bundler-2.3.0/lib/bundler/vendor/thor/lib/thor/error.rb:105: warning: constant DidYouMean::SPELL_CHECKERS is deprecated
    Calling `DidYouMean::SPELL_CHECKERS.merge!(error_name => spell_checker)' has been deprecated. Please call `DidYouMean.correct_error(error_name, spell_checker)' instead.
    C:/Ruby32-x64/lib/ruby/gems/3.2.0/gems/serverengine-2.3.1/lib/serverengine/socket_manager_win.rb:140: warning: assigned but unused variable - type
    C:/Ruby32-x64/lib/ruby/gems/3.2.0/gems/cool.io-1.7.1/lib/iobuffer_ext.so: warning: method redefined; discarding old initialize
    C:/Ruby32-x64/lib/ruby/gems/3.2.0/gems/cool.io-1.7.1/lib/iobuffer_ext.so: warning: method redefined; discarding old clear
    C:/Ruby32-x64/lib/ruby/gems/3.2.0/gems/cool.io-1.7.1/lib/iobuffer_ext.so: warning: method redefined; discarding old empty?
    C:/Ruby32-x64/lib/ruby/gems/3.2.0/gems/cool.io-1.7.1/lib/iobuffer_ext.so: warning: method redefined; discarding old write
    C:/Ruby32-x64/lib/ruby/gems/3.2.0/gems/fluentd-1.15.3-x64-mingw-ucrt/lib/fluent/plugin_helper.rb:46: warning: method redefined; discarding old inherited
    C:/Ruby32-x64/lib/ruby/gems/3.2.0/gems/fluentd-1.15.3-x64-mingw-ucrt/lib/fluent/plugin_helper.rb:46: warning: previous definition of inherited was here
    Loaded suite C:/Ruby32-x64/lib/ruby/gems/3.2.0/gems/rake-13.0.6/lib/rake/rake_test_loader
    Started
    F
    ==============================================================================================================================================================================================================================================
          assert { out.include?("count:20") }
                   |   |
                   |   false
                   ""
    C:/Users/reang/Documents/work/fluentd/fluentdPlugins/fluent-plugin-flowcounter-simple/test/plugin/test_filter_flowcounter_simple.rb:30:in `test_filter'
         27:     d = create_driver
         28:     filtered, out = filter(d, msgs)
         29:     assert_equal msgs, filtered
      => 30:     assert { out.include?("count:20") }
         31:   end
         32:
         33:   private
    ==============================================================================================================================================================================================================================================....
    Finished in 0.5054875 seconds.
    ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------5 tests, 9 assertions, 1 failures, 0 errors, 0 pendings, 0 omissions, 0 notifications
    ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------9.89 tests/s, 17.80 assertions/s
    Command failed with status (1): [ruby -w -I"lib;lib;test" C:/Ruby32-x64/lib/ruby/gems/3.2.0/gems/rake-13.0.6/lib/rake/rake_test_loader.rb "test/plugin/test_filter_flowcounter_simple.rb" "test/plugin/test_out_flowcounter_simple.rb" ]
    
    Tasks: TOP => default => test
    (See full trace by running task with --trace)

@daipom daipom mentioned this pull request Feb 22, 2023
@kou
Copy link

kou commented Apr 4, 2023

Could you improve the title of this pull request something like test: don't use deprecated capture_log?

@daipom daipom changed the title test: fix logger handling test: don't use deprecated capture_log Apr 5, 2023
@daipom
Copy link
Contributor Author

daipom commented Apr 5, 2023

Could you improve the title of this pull request something like test: don't use deprecated capture_log?

Thanks! That is more specific and easy to understand. Fixed.

@daipom
Copy link
Contributor Author

daipom commented Apr 5, 2023

I just rebased this branch to the latest master.

@daipom daipom changed the title test: don't use deprecated capture_log test: don't use deprecated way of capturing logs Apr 5, 2023
@daipom
Copy link
Contributor Author

daipom commented Apr 5, 2023

I thought that capture_log was the method of Fluentd, but it was wrong.

This method is defined by this plugin in helper.rb.

class Test::Unit::TestCase
def capture_log(log)
if defined?(Fluent::Test::TestLogger) and log.is_a?(Fluent::Test::TestLogger) # v0.14
yield
log.out.logs.join("\n")
else
begin
tmp = log.out
log.out = StringIO.new
yield
return log.out.string
ensure
log.out = tmp
end
end
end
end

But this is very similar to the Fluentd method and should not be used.

I changed the title a little, and also fixed the helper.rb.

test/helper.rb Outdated Show resolved Hide resolved
test/helper.rb Outdated Show resolved Hide resolved
test/helper.rb Outdated Show resolved Hide resolved
`capture_log` is for older versions as well as the following.

https://github.com/fluent/fluentd/blob/6c649d189b5cf65ccfce58f6952a31b24c775e72/lib/fluent/test/helpers.rb#L110-L122

We should use `Driver::logs` instead.

In addition, this fixes the following points.

* Update `helper.rb` since it was updated very long ago and there
  are some unnecessary codes.
* Assert the entire log message (without the timestamp and the
  log level), since this is easier to see the diff of the expected
  and the actual when the assertion fails.

Signed-off-by: Daijiro Fukuda <[email protected]>
@daipom
Copy link
Contributor Author

daipom commented Apr 6, 2023

I have removed unnecessary codes from test/helper.rb.

Copy link

@kou kou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

@daipom daipom merged commit 7059ee2 into fluent-plugins-nursery:master Apr 6, 2023
@daipom daipom deleted the fix-test branch April 6, 2023 05:34
@daipom
Copy link
Contributor Author

daipom commented Apr 6, 2023

Thanks for your review!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants