Skip to content
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

test: use Fluent::Config::Element in test #4074

Merged
merged 1 commit into from
Mar 7, 2023

Conversation

abetomo
Copy link
Contributor

@abetomo abetomo commented Feb 28, 2023

Which issue(s) this PR fixes:

( #4066 (comment) )

What this PR does / why we need it:

There is a case where Hash is used in the test of configure method.
I fixed it because configure method should use Fluent::Config::Element.

Docs Changes:

Release Note:

We should use `Fluent::Config::Element` instead of Hash.

Signed-off-by: abetomo <[email protected]>
@ashie
Copy link
Member

ashie commented Mar 3, 2023

Whether we should merge this or not depends on the discussion in #4066 (comment)

@daipom
Copy link
Contributor

daipom commented Mar 3, 2023

Sorry for the delay, yes I think so too.

I'm considering #4066's goal.
Please give me more time.

@ashie
Copy link
Member

ashie commented Mar 7, 2023

Whether we should merge this or not depends on the discussion in #4066 (comment)

Our conclusion in the discussion is #4066 (comment)

So, I think the correct specification is as follows.

* Only the `Fluent::Config::Element` should be passed to `Fluent::Plugin::Base::configure()`.

* All tests passing `Hash` directly to `Fluent::Plugin::Base::configure()` are wrong and must use `TestDriver`.

So we'll merge this.

@ashie ashie merged commit c0125d8 into fluent:master Mar 7, 2023
@ashie
Copy link
Member

ashie commented Mar 7, 2023

Thanks!

@@ -135,7 +139,7 @@ def setup
end

def test_format
@formatter.configure({})
@formatter.configure(config_element())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this is the one we should use Fluent::Test::FormatterTestDriver for.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, sorry I've already merged this although you are right...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No problem! I'm making another PR for this now!

@ashie ashie added this to the v1.16.0 milestone Mar 7, 2023
@abetomo abetomo deleted the use-config_element-in-tests branch March 7, 2023 01:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants