Apply newer service_discovery plugin helper methods#3362
Merged
Conversation
Signed-off-by: TAGOMORI Satoshi <tagomoris@gmail.com>
Member
Author
|
I'll merge it after CI is completed. |
Member
Author
Hmm, 2 tests are failed on all platforms 🤔 |
Plugin type is a string value, so the test should exepect a string value while the previous out_forward code pass a symbol value as a service_discovery plugin type by one-off code. Signed-off-by: Takuro Ashie <ashie@clear-code.com>
A test should ensure to have only one assertion because trailing
assertions aren't evalueated when an assertions is failed. It makes
debugging hard. Here is an example output of the test:
before:
Failure: test: server is an abbreviation of static type of service_discovery(ForwardOutputTest)
/home/aho/Projects/Fluentd/fluentd/test/plugin/test_out_forward.rb:282:in `block in <class:ForwardOutputTest>'
<1234> expected but was
<1235>
diff:
? 1234
? 5
after:
Failure: test: server is an abbreviation of static type of service_discovery(ForwardOutputTest)
/home/aho/Projects/Fluentd/fluentd/test/plugin/test_out_forward.rb:281:in `block in <class:ForwardOutputTest>'
<[{:host=>"127.0.0.1", :port=>1234}, {:host=>"127.0.0.1", :port=>1235}]> expected but was
<[{:host=>"127.0.0.1", :port=>1235}, {:host=>"127.0.0.1", :port=>1234}]>
diff:
? [{:host=>"127.0.0.1", :port=>1234}, {:host=>"127.0.0.1", :port=>1235}]
? 5 4
The later one is easy to understand that the array is inverted.
Signed-off-by: Takuro Ashie <ashie@clear-code.com>
Member
Author
|
BTW although tests in fluentd tend to check arrays by separated assertions like this: assert_equal 2, d.instance.discovery_manager.services.size
assert_equal '127.0.0.1', d.instance.discovery_manager.services[0].host
assert_equal 1234, d.instance.discovery_manager.services[0].port
assert_equal '127.0.0.1', d.instance.discovery_manager.services[1].host
assert_equal 1235, d.instance.discovery_manager.services[1].portI don't like such tests because it makes hard to debug. assert_equal(
[
{ host: '127.0.0.1', port: 1234 },
{ host: '127.0.0.1', port: 1235 },
],
d.instance.discovery_manager.services.collect do |service|
{ host: service.host, port: service.port }
end
)Here is an example output: before: after: The later one is easy to understand that the array is inverted. |
"server" directive should have higher priority than "service" in "service_discovery" to keep backward compatibility. Signed-off-by: Takuro Ashie <ashie@clear-code.com>
Member
Author
|
tests passed (without a known unstable test). |
Member
Member
Author
|
Don't worry 😃 |
Member
|
That looks perfect! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which issue(s) this PR fixes:
none
What this PR does / why we need it:
Apply a new helper method added at #3299 to out_forward.
This PR is revised version of #3300 which was closed unexpectedly.
Docs Changes:
Same with #3299
Release Note:
Same with #3299