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

Use rr instead of flexmock #790

Closed
wants to merge 9 commits into from

Conversation

cosmo0920
Copy link
Contributor

I try to use rr instead of flexmock in tests.
In most cases, using rr is slightly readable than flexmock.

Current status

  • test_filter_stdout
  • test_output
  • test_in_tail
  • use mock instead of stub as much as possible

Question

  • Should we completely replace flexmock with rr?
    • test_z_refresh_watchers's test case is hard to replace flexmock with rr. → Done.


flexstub(Fluent::NewTailInput::TailWatcher) do |watcherclass|
stub(Fluent::NewTailInput::TailWatcher) do |watcherclass|
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This test case does not replace flexmock with rr completely.
For now, work in progress.

Copy link
Contributor

Choose a reason for hiding this comment

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

What is the problem?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What is the problem?

Here is a test failure log:

test_z_refresh_watchers:                                                                        F
=================================================================================================================================================================================================
Failure:
  new("test/plugin/data/2010/01/20100102-030406.log", 0, #<RR::DoubleDefinitions::DoubleDefinition:0x007fd273d4c4c8>, anything, true, 1000, anything, anything, anything)
  Called 0 times.
  Expected 1 times.
test_z_refresh_watchers(TailInputTest)
/Users/hhatake/Github/fluentd/test/plugin/test_in_tail.rb:514:in `block (2 levels) in test_z_refresh_watchers'
     511: 
     512:       stub(Fluent::NewTailInput::TailWatcher) do |watcherclass|
     513:         file_position_entry = stub(Fluent::NewTailInput::FilePositionEntry)
  => 514:         watcherclass.new('test/plugin/data/2010/01/20100102-030406.log', EX_RORATE_WAIT, file_position_entry, anything, true, 1000, anything, anything, anything).once do
     515:           mock('TailWatcher') do |watcher|
     516:             watcher.attach(anything).once
     517:             watcher.__send__(:unwatched=).at_least(0)
/Users/hhatake/Github/fluentd/test/plugin/test_in_tail.rb:512:in `block in test_z_refresh_watchers'
/Users/hhatake/Github/fluentd/test/plugin/test_in_tail.rb:490:in `test_z_refresh_watchers'
=================================================================================================================================================================================================
: (0.022009)

Copy link
Contributor

Choose a reason for hiding this comment

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

Your usage is wrong. Use the following patch:

diff --git a/test/plugin/test_in_tail.rb b/test/plugin/test_in_tail.rb
index d9b6086..80dceca 100644
--- a/test/plugin/test_in_tail.rb
+++ b/test/plugin/test_in_tail.rb
@@ -488,19 +488,33 @@ class TailInputTest < Test::Unit::TestCase
     end

     stub(Time) do |timeclass|
-      timeclass.now.returns(Time.new(2010, 1, 2, 3, 4, 5), Time.new(2010, 1, 2, 3, 4, 6), Time.new(2010, 1, 2, 3, 4, 7))
+      nows = [
+        Time.new(2010, 1, 2, 3, 4, 5),
+        Time.new(2010, 1, 2, 3, 4, 6),
+        Time.new(2010, 1, 2, 3, 4, 7),
+      ]
+      timeclass.now do
+        nows.shift
+      end

       stub(Fluent::NewTailInput::TailWatcher) do |watcherclass|
         EX_PATHS.each do |path|
-          file_position_entry = stub(Fluent::NewTailInput::FilePositionEntry)
-          watcherclass.new(path, EX_RORATE_WAIT, file_position_entry, anything, true, 1000, anything, anything).once do
+          new = watcherclass.new(path,
+                                 EX_RORATE_WAIT,
+                                 is_a(Fluent::TailInput::FilePositionEntry),
+                                 anything,
+                                 true,
+                                 1000,
+                                 anything,
+                                 anything) do
             mock('TailWatcher') do |watcher|
               watcher.attach(anything).once
-              watcher.__send__(:unwatched=).at_least(0)
+              watcher.__send__(:unwatched=, true).at_least(0)
               watcher.line_buffer.at_least(0)
               watcher.close.at_least(0)
             end
           end
+          new.once
         end
         plugin.refresh_watchers
       end
@@ -510,14 +524,21 @@ class TailInputTest < Test::Unit::TestCase
       end

       stub(Fluent::NewTailInput::TailWatcher) do |watcherclass|
-        file_position_entry = stub(Fluent::NewTailInput::FilePositionEntry)
-        watcherclass.new('test/plugin/data/2010/01/20100102-030406.log', EX_RORATE_WAIT, file_position_entry, anything, true, 1000, anything, anything, anything).once do
+        new = watcherclass.new('test/plugin/data/2010/01/20100102-030406.log',
+                               EX_RORATE_WAIT,
+                               is_a(Fluent::TailInput::FilePositionEntry),
+                               anything,
+                               true,
+                               1000,
+                               anything,
+                               anything) do
           mock('TailWatcher') do |watcher|
             watcher.attach(anything).once
-            watcher.__send__(:unwatched=).at_least(0)
+            watcher.__send__(:unwatched=, true).at_least(0)
             watcher.line_buffer.at_least(0)
           end
         end
+        new.once
         plugin.refresh_watchers
       end
  1. timeclass.now.returns(now1, now2, now3) doesn't define a stub that returns now1, then now2 and then now3. The stub always returns now1.
  2. stub(Fluent::NewTailInput::FilePositionEntry) doesn't mean that any FilePositionEntry instances. Use is_a(Fluent::TailInput::FilePositionEntry).
  3. watcherclass.new.once do ... end is wrong. do ... end is a block for once not new.
  4. watcher.__send__(:unwatched=).at_least(0) misses an expected argument of #unwatched=.
  5. The number of arguments of the second watcherclass.new is wrong. It passes 9 arguments but new is called with 8 arguments.

By the way, you should use mock instead of stub to check expected behavior. stub is used just for stub. It means that stub just provides features for running a test. It doesn't check anything. mock is used for checking behaviors. Search "stub mock difference" on Web search engine for details.

@cosmo0920 cosmo0920 force-pushed the use-rr-instead-of-flexmock branch from d987937 to 9ea787c Compare February 12, 2016 02:49
@cosmo0920 cosmo0920 force-pushed the use-rr-instead-of-flexmock branch from 066891e to 433261b Compare February 12, 2016 10:14
@cosmo0920 cosmo0920 changed the title [WIP] Use rr instead of flexmock Use rr instead of flexmock Feb 15, 2016
@tagomoris
Copy link
Member

Replacing flexmock with rr simply looks not good for me.
Anyway, it's a bit late to reconsider this point, and now we have a problem about rr.
#1115

@tagomoris tagomoris closed this Jul 21, 2016
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.

3 participants