logstash-plugins / logstash-filter-multiline

Apache License 2.0
18 stars 16 forks source link

integration and regression spec for Java ArrayList merge #7

Open colinsurprenant opened 9 years ago

colinsurprenant commented 9 years ago

This is a regression spec for elasticsearch/logstash#2372

This spec does not pass on version < 0.1.4 and is now fixed in 0.1.4

jordansissel commented 9 years ago

:ballot_box_with_check: Code review I haven't run the tests yet.

jsvd commented 9 years ago

Running the tests I get:

Run options: exclude {:redis=>true, :socket=>true, :performance=>true, :couchdb=>true, :elasticsearch=>true, :elasticsearch_secure=>true, :broken=>true, :export_cypher=>true, :integration=>true}
.F.........

Failures:

  1) LogStash::Filters::Multiline integrations should merge messages arrays as Java ArrayList from json codec using JrJackson
     Failure/Error: results += filter.flush(:final => true)
     ArgumentError:
       wrong number of arguments (1 for 3)
     # ./lib/logstash/filters/multiline.rb:271:in `merge'
     # ./lib/logstash/filters/multiline.rb:270:in `merge'
     # ./lib/logstash/filters/multiline.rb:204:in `flush'
     # ./lib/logstash/filters/multiline.rb:202:in `flush'
     # ./spec/filters/multiline_spec.rb:299:in `(root)'

Finished in 1.42 seconds
11 examples, 1 failure

Failed examples:

rspec ./spec/filters/multiline_spec.rb:251 # LogStash::Filters::Multiline integrations should merge messages arrays as Java ArrayList from json codec using JrJackson

Randomized with seed 44754
colinsurprenant commented 9 years ago

@jsvd could you re test please?

jsvd commented 9 years ago

Still getting the same error, that comes from within this cycle:

data = events.inject({}) do |result, event|
  self.class.event_hash_merge!(result, event.to_hash_with_metadata, dups_key)
end
purbon commented 9 years ago

@colinsurprenant is this PR still valid? can we fix the test somehow as @jsvd proposed?

talevy commented 8 years ago

@colinsurprenant @jsvd friendly ping to see what the status on this PR is

colinsurprenant commented 7 years ago

ok, I'll reassess the need for this PR - it's been lingering for so long.