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

fix unstable test_in_tail #957

Merged
merged 1 commit into from
May 16, 2016
Merged

fix unstable test_in_tail #957

merged 1 commit into from
May 16, 2016

Conversation

sonots
Copy link
Member

@sonots sonots commented May 16, 2016

@sonots sonots merged commit 483e46f into master May 16, 2016
@sonots sonots deleted the fix_test_in_tail branch May 16, 2016 12:18
@tagomoris
Copy link
Member

Is this test the problem correctly?
I think that it should do these:

  • tail1.txt appears twice
  • tail2.txt appears twice
  • (optional?) lines from same file should 0-1 & 2-3.

Tests should confirm the sources of events.

@sonots
Copy link
Member Author

sonots commented May 16, 2016

Not directly writing, but this does test both tail1.txt and tail2.txt appears twice.

@tagomoris
Copy link
Member

I meant that it should check that the sources of these records are tail1.txt and tail2.txt explicitly.

@sonots
Copy link
Member Author

sonots commented May 16, 2016

like this?

diff --git a/test/plugin/test_in_tail.rb b/test/plugin/test_in_tail.rb
index a0c1b6a..80818d1 100644
--- a/test/plugin/test_in_tail.rb
+++ b/test/plugin/test_in_tail.rb
@@ -908,10 +908,10 @@ class TailInputTest < Test::Unit::TestCase
         files.each do |file|
           File.open(file, 'ab') { |f|
             f.puts "f #{file} line should be ignored"
-            f.puts "s test1"
+            f.puts "s #{file}"
             f.puts "f test2"
             f.puts "f test3"
-            f.puts "s test4"
+            f.puts "s #{file}"
           }
         end
         sleep 1
@@ -920,8 +920,12 @@ class TailInputTest < Test::Unit::TestCase
       emits = d.emits
       assert(emits.length == 4)
       assert_equal(files, [emits[0][2]["path"], emits[1][2]["path"]].sort)
-      # "test4" events are here because these events are flushed at shutdown phase
+      assert_equal(emits[0][2]["path"], emits[0][2]["message"].lines.first.chomp)
+      assert_equal(emits[1][2]["path"], emits[1][2]["message"].lines.first.chomp)
+      # last line events are here because these events are flushed at shutdown phase
       assert_equal(files, [emits[2][2]["path"], emits[3][2]["path"]].sort)
+      assert_equal(emits[2][2]["path"], emits[2][2]["message"].lines.first.chomp)
+      assert_equal(emits[3][2]["path"], emits[3][2]["message"].lines.first.chomp)
     end
   end
 end

@tagomoris
Copy link
Member

No. It's incorrect because these paths might be different one from unknown files which are used in other tests and not cleaned up.
This tests should be like this:

paths = emits.map{|e| e[2]["path"] }
assert{ paths.select{|path| path == "#{TMP_DIR}/tail1.txt" }.size == 2 }
assert{ paths.select{|path| path == "#{TMP_DIR}/tail2.txt" }.size == 2 }

@sonots
Copy link
Member Author

sonots commented May 16, 2016

Well, I want to test last lines (which are flushed on shutdown

record[@path_key] ||= tw.path unless @path_key.nil?
) and non-last lines (which are emitted when next first lines are appeared
record[@path_key] ||= tail_watcher.path unless @path_key.nil?
) separately since they are treated in different lines of codes although this is an optional test.

assert_equal(files, [emits[0][2]["path"], emits[1][2]["path"]].sort)
assert_equal(files, [emits[2][2]["path"], emits[3][2]["path"]].sort)

expresses almost same tests with your tests in addition to the option.

@sonots
Copy link
Member Author

sonots commented May 16, 2016

I mean what I want to do is like below, but this is almost same with the current test codes

paths = emits[0..1].map{|e| e[2]["path"] }
assert{ paths.select{|path| path == "#{TMP_DIR}/tail1.txt" }.size == 1 }
assert{ paths.select{|path| path == "#{TMP_DIR}/tail2.txt" }.size == 1 }
# last line events are here because these events are flushed at shutdown phase
paths = emits[2..3].map{|e| e[2]["path"] }
assert{ paths.select{|path| path == "#{TMP_DIR}/tail1.txt" }.size == 1 }
assert{ paths.select{|path| path == "#{TMP_DIR}/tail2.txt" }.size == 1 }

@tagomoris
Copy link
Member

Ah, okay, got it > "records flushed at shutdown"
I think it's better to separate test case for normal case and records flushed at shutdown though.

repeatedly pushed a commit that referenced this pull request May 19, 2016
@repeatedly repeatedly added v0.12 bug Something isn't working v0.14 labels May 19, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working v0.12 v0.14
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants