-
Notifications
You must be signed in to change notification settings - Fork 16
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 ECS test failure cases. #23
Fix ECS test failure cases. #23
Conversation
spec/inputs/stdin_spec.rb
Outdated
|
||
subject { LogStash::Inputs::Stdin.new } | ||
require "logstash/codecs/json" | ||
subject { LogStash::Inputs::Stdin.new("codec" => LogStash::Codecs::JSON.new) } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
review note why codec change needs: the root cause for test failure is, we ca calling directly process()
method of the plugin which \n
always stay. For the real scenarios, StdinChannel::Reader.new
or standard $stdin
escapes the new line. So, in the test cases, changed plugin default Line
(since its delimiter \n
and removes when ECS enabled) codec to JSON
codec to avoid the new line (\n
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of changing the codecs, maybe we can add strip
to the validation
expect( event.get('event') ).to eql 'original' => stdin_data.strip
just like the "sets hostname" test case
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mashhurs Thanks for improving the test cases. I have one suggestion to make the test case aligned with the others.
spec/inputs/stdin_spec.rb
Outdated
|
||
subject { LogStash::Inputs::Stdin.new } | ||
require "logstash/codecs/json" | ||
subject { LogStash::Inputs::Stdin.new("codec" => LogStash::Codecs::JSON.new) } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of changing the codecs, maybe we can add strip
to the validation
expect( event.get('event') ).to eql 'original' => stdin_data.strip
just like the "sets hostname" test case
@kaisecheng if we align on default
and I have applied it. Details: we receive the data with |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Description
This change fixes the test case failure, example failure.
Details: the root cause for test failure is, we ca calling directly
process()
method of the plugin. For the real scenarios,StdinChannel::Reader.new
or standard$stdin
escapes the new line. So, in the test cases, changed plugin defaultLine
codec toJSON
codec to avoid the new line (\n
).