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

Add ParserTestDriver #702

Merged
merged 16 commits into from
Nov 25, 2015
Merged

Conversation

cosmo0920
Copy link
Contributor

Currently, fluentd does not have ParserTestDriver.
This PR adds this test driver.

Remaining tasks

  • Frozen ParserTestDriver API
    • Decide whether to include utility methods such as str2time or not.
  • Replace a few parser test cases with ParserTestDriver
  • Documentations?

@repeatedly
Copy link
Member

Could you replace one actual parser test with this driver?

@cosmo0920
Copy link
Contributor Author

I try to replace TimeParser and SyslogParse test cases with ParserTestDriver.

@cosmo0920
Copy link
Contributor Author

I've found that parser test driver in this commit cosmo0920@d5d0846 does not handle to instantiate RegexParser.
How do we abstract this parser with this test driver?

Ah, my understanding is wrong. In RegexParser#initialize, conf argument is optional...

@okkez
Copy link
Contributor

okkez commented Nov 16, 2015

I want to write parser test like following:

def test_multiline
  conf = %[
  <grok>
    pattern %{DATA:message1}\n
  </grok>
  <grok>
    pattern %{DATA:message2}\n
  </grok>
  ]
  text = <<TEXT
host1 message1
  message2
TEXT
  d = Fluent::Parser::TestDriver.new(Fluent::TextParser::MultilineGrokParser).configure(conf)
  d.parse(text) do |time, record|
     assert_equal(expected, record)
  end
end

This is same as other test drivers.

@repeatedly
Copy link
Member

TimeParser itself is not a Parser plugin so I think supporting TimeParser API is not needed.

@cosmo0920
Copy link
Contributor Author

Rebased. I removed a commit which replaces TimeParse test cases with ParserTestDriver.

@repeatedly
Copy link
Member

@okkez How about this? If you ok, I will merge this patch.

@okkez
Copy link
Contributor

okkez commented Nov 18, 2015

How to use nested config_section and cnofig_param?
How about this?

But this is no tested.

def configure(conf)
  case conf
  when Fluent::Config::Element
    @config = conf
  when String
    io = StringIO.new(conf)
    @config = Cofig::Parser.parse(io, 'fluent.conf')
  when Hash
    @config = Config::Element.new('ROOT', '', conf, [])
  else
    raise "Unknown type... #{conf}"
  end
  @instance.configure(@config)
  self
end

@cosmo0920
Copy link
Contributor Author

As a result of @okkez and I discussion, I added commits to drop TimeParser support in this test driver, to fix usage of Class.new, and to support string literal in #configure.

Could you review this PR again?

@cosmo0920 cosmo0920 changed the title [WIP] Add ParserTestDriver Add ParserTestDriver Nov 19, 2015
@repeatedly
Copy link
Member

If there are no concerns from other developers, I will merge it.

@okkez
Copy link
Contributor

okkez commented Nov 24, 2015

Looks good to me 👍

repeatedly added a commit that referenced this pull request Nov 25, 2015
@repeatedly repeatedly merged commit 480d0b2 into fluent:master Nov 25, 2015
@cosmo0920 cosmo0920 deleted the add-parser-test-driver branch November 25, 2015 17:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants