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.

Sign up for GitHub

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

Jump to bottom

Use driver for RegexpParser tests #4085

Merged
merged 1 commit into from
Mar 9, 2023
Merged

Use driver for RegexpParser tests #4085

merged 1 commit into from
Mar 9, 2023

Conversation

abetomo
Copy link
Contributor

@abetomo abetomo commented Mar 9, 2023

Which issue(s) this PR fixes:

( #4066 (comment) )

What this PR does / why we need it:

I didn't know why it was commented out, but it looked good on Fluent::Test::Driver::Parser.new(...).
#1286

Docs Changes:

None

Release Note:

None

@daipom daipom self-requested a review March 9, 2023 01:16
daipom
daipom approved these changes Mar 9, 2023
View reviewed changes
Copy link
Contributor

@daipom daipom left a comment

Choose a reason for hiding this comment

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

Thanks! We should fix tests like this.

Certainly, the intent of the #1286 fix is unclear.

Fluent::Test::Driver::Parser::configure() calls instance.configure(), so I think the old code is meaningless.

I have confirmed the test succeeds even on #1286 with this fix.

It looks good to me.

@ashie ashie merged commit 8812ec1 into fluent:master Mar 9, 2023
@ashie
Copy link
Member

ashie commented Mar 9, 2023

Thanks!

@ashie ashie added this to the v1.16.0 milestone Mar 9, 2023
@abetomo abetomo deleted the use-driver-for-parser_regexp-tests branch March 9, 2023 03:36
@abetomo
Copy link
Contributor Author

abetomo commented Mar 9, 2023

Thanks for the merge!

@renovate renovate bot mentioned this pull request Apr 17, 2023
Open
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@daipom daipom daipom approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v1.16.0
Development

Successfully merging this pull request may close these issues.
3 participants
@abetomo @ashie @daipom