You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
In fixing issue #1160 the PR #1161 added some testing to cover the case of integration metadata in combination with datastreams.
This led to the the creation of two tests which switched method under-test:
In these tests the method under test switched from event_action_tuple to data_stream_event_action_tuple to test the datastream scenario.
It would mean that abstraction level tested by test isn't right one, ideally the fixture's setup would trigger one path of execution or the other. There shouldn't be knowledge in the test about which method to call, in this context.
This issue is proposing to introduce a change so that the tests haven't knowldege to call event_action_tuple or data_stream_event_action_tuple to proper test the use case, should be exposed a more abstract method which isolate from this decision, and decision is made only based on the data in the fixture.
Possibile solution could be the introduction o action_tuple(event) that uses the @event_mapper (which presently wraps either event_action_tuple or data_stream_event_action_tuple depending on other configuration):
And then changing the current only call-site of @event_mapper.call to go through our new unified boundary:
diff --git a/lib/logstash/outputs/elasticsearch.rb b/lib/logstash/outputs/elasticsearch.rb
index 598265d..55e9a87 100644
--- a/lib/logstash/outputs/elasticsearch.rb+++ b/lib/logstash/outputs/elasticsearch.rb@@ -424,7 +424,7 @@ class LogStash::Outputs::ElasticSearch < LogStash::Outputs::Base
event_mapping_errors = [] # list of FailedEventMapping
events.each do |event|
begin
- successful_events << @event_mapper.call(event)+ successful_events << action_tuple(event)
rescue EventMappingError => ie
event_mapping_errors << FailedEventMapping.new(event, ie.message)
end
We could then test action_tuple directly for a given plugin config without needing to know whether we should be testing event_action_tuple or data_stream_event_action_tuple.
Another possible way would be to pull the datastream-specific and legacy-specific behaviour out into a self-contained Modules so that setup_mapper_and_target wouldn't need to store procs-that-call-specific-methods.
The text was updated successfully, but these errors were encountered:
In fixing issue #1160 the PR #1161 added some testing to cover the case of integration metadata in combination with datastreams.
This led to the the creation of two tests which switched method under-test:
logstash-output-elasticsearch/spec/unit/outputs/elasticsearch_spec.rb
Lines 310 to 312 in 77ca162
logstash-output-elasticsearch/spec/unit/outputs/elasticsearch_spec.rb
Lines 339 to 341 in 77ca162
In these tests the method under test switched from
event_action_tuple
todata_stream_event_action_tuple
to test the datastream scenario.It would mean that abstraction level tested by test isn't right one, ideally the fixture's setup would trigger one path of execution or the other. There shouldn't be knowledge in the test about which method to call, in this context.
This issue is proposing to introduce a change so that the tests haven't knowldege to call
event_action_tuple
ordata_stream_event_action_tuple
to proper test the use case, should be exposed a more abstract method which isolate from this decision, and decision is made only based on the data in the fixture.Possibile solution could be the introduction o
action_tuple(event)
that uses the@event_mapper
(which presently wraps eitherevent_action_tuple
ordata_stream_event_action_tuple
depending on other configuration):And then changing the current only call-site of
@event_mapper.call
to go through our new unified boundary:We could then test
action_tuple
directly for a given plugin config without needing to know whether we should be testingevent_action_tuple
ordata_stream_event_action_tuple
.Another possible way would be to pull the datastream-specific and legacy-specific behaviour out into a self-contained Modules so that
setup_mapper_and_target
wouldn't need to store procs-that-call-specific-methods.The text was updated successfully, but these errors were encountered: