magnusbaeck / logstash-filter-verifier

Apache License 2.0
192 stars 27 forks source link

Testcaseset (have a testcase in one hash) #21

Closed breml closed 7 years ago

breml commented 7 years ago

Added testcases section to the test case file

The test case file now supports an additional section called testcases, which consists of an input line and the expected event, after processed by Logstash.

This allows for clean test case files, as the actual input is just next to the expected output, which makes working with the test case files much easier.

Renamed TestCase to TestCaseSet

Renamed TestCase to TestCaseSet in preparation for the additon of the extended testcase feature in the config files. (separate commit).

Future improvements

The new testcases struct could be further extended, for example with the following fields:

breml commented 7 years ago

At a later point I would consider to deprecate the fields input and expected and only use the newly added testcase hash.

matejzero commented 7 years ago

I would LOVE to see this implemented/merged.

Sometimes, input lines are almost identical, but the result is very different. On other cases, I have a lot of different test lines in one test file and it's really hard to manage.

breml commented 7 years ago

I just updated the PR based on your feedback and merged master to resolve the merge conflicts.

Regarding the testcases involving multiple lines I think in the long run I personally would prefer to make TestCaseSet to support multiple lines, and to deprecate the legacy input and expected fields.

@magnusbaeck if you agree, I offer to update/extend the PR.

magnusbaeck commented 7 years ago

Regarding the testcases involving multiple lines I think in the long run I personally would prefer to make TestCaseSet to support multiple lines, and to deprecate the legacy input and expected fields.

Sounds good. Let's add support for multiline TestCaseSet now and aim for deprecation of the legacy options in a 2.0 release.

if you agree, I offer to update/extend the PR.

Yes, please!

breml commented 7 years ago

@magnusbaeck I had an other look at the filter plugins you mentioned in https://github.com/magnusbaeck/logstash-filter-verifier/pull/21#pullrequestreview-19883505 and also looked at some other special cases.

  1. drop: I added a check, If the ExpectedEvent is null (json-value), an empty array [] or an empty map {}, in these cases, the element is not added to the slice of ExpectedEvents. This allows to test, if the drop filter (or any event.cancel is working.

  2. Intervall based filters like metrics: this kind of filters are currently (with or without this PR) not possible to test. The problem is, that in the case of metrics no flush is performed, if the Logstash pipeline is shutdown. This means, that for the test to be successful, the config of the filter would need to have an interval configured, such that exactly one flush is performed. This is very unstable and therefore unlikely to work reliable. A possible solution would be, to provide an timeout to logstash-filter-verifier for a minimal runtime as well as an option for the testcases to ignore events after x events are received (example: only evaluate the first event received, ignore all other, so only one metric event would be evaluated).

  3. clone allow to add more events to the pipeline, where one event could result in multiple ExpectedEvent. Therefore for the TestCaseSet, ExpectedEvent also needs to be a slice.

magnusbaeck commented 7 years ago

Excellent! We can figure out the metrics filter later on. I'll look through all patches this weekend.

magnusbaeck commented 7 years ago

Branch rebased and merged to master.