Closed vervas closed 8 years ago
CLA is signed
@vervas thanks a lot for your contribution, can you elaborate why this change is necessary and/or interesting?
Hey @purbon the problem that I see here is that when calling https://github.com/logstash-plugins/logstash-input-gelf/blob/master/lib/logstash/inputs/gelf.rb#L128 one expects to get back a LogStash::Event
. Tests declare to validate this, although this path is never executed since it is "mocked" with the introduction of https://github.com/logstash-plugins/logstash-input-gelf/commit/c28c580e7e1e7b41f70c5fe229cd1e1d3ad2733a#diff-b15c84752b9b4e665a93451d2d58a0baR153. An easy way to observe this is by adding a puts
statement in both functions and even better ensuring that either one path or another is tested, by adding the following block in each respective test case like:
it "should call 'from_json_parse'" do
expect(subject).to receive(:from_json_parse)
end
Eventually when from_json_parse
is called one shall expect to receive one event, however the each
function (seen at https://github.com/logstash-plugins/logstash-input-gelf/blob/master/lib/logstash/inputs/gelf.rb#L151) returns the array itself back. This is definitely not the expected behaviour and it cannot be handled properly.
Unfortunately, in my understanding, what is going to be executed during the tests, depends on the environment, and the results vary. This is also an issue which I'm not sure how it can be handled in a way that every possible scenario is covered.
@purbon Is this something you could review or shall we put others in the loop to evaluate it?
Thanks for contributing to Logstash! If you haven't already signed our CLA, here's a handy link: https://www.elastic.co/contributor-agreement/