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

Testcaseset (have a testcase in one hash) #21

Closed
wants to merge 4 commits into from

Conversation

breml
Copy link
Collaborator

@breml breml commented Jan 27, 2017

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:

  • json_lines: as an alternative way to provide the input, which would remove the need for the escaping of json input in the input field.
  • ignore_fields: fields, that should be ignored just for the actual test case, additionally to the globally defined ignored fields.
  • description: short description field for the test case, which would allow to describe, why the test case was added, e.g. with reference to ticket describing the bug/issue. This description could be printed while running the tests instead of (or additional to) the current outout: Comparing message <n> of <filename>...

in preparation for the additon of the extended testcase feature
in the config files.
@breml breml changed the title Testcaseset Testcaseset (have a testcase in one hash) Jan 27, 2017
@breml
Copy link
Collaborator Author

breml commented Jan 27, 2017

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

@matejzero
Copy link

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.

Copy link
Owner

@magnusbaeck magnusbaeck left a comment

Choose a reason for hiding this comment

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

This makes total sense. I just wonder how we keep support for testcases involving multiple lines. While the multiline filter has been deprecated there are other filters like aggregate and collate that rely on multiple input lines and that should be testable. I suppose we could either

  • make TestCaseSet support multiple input lines and expected events or
  • keep the legacy input and expected fields around for multiline needs.

The former would allow us to (eventually) remove input and expected but would also add an extra layer of arrays just to support an unusual use case.

@@ -368,10 +368,10 @@ func TestMarshalToFile(t *testing.T) {
}
}

func marshalTestCase(t *testing.T, tc *TestCase) string {
resultBuf, err := json.MarshalIndent(tc, "", " ")
func marshalTestCase(t *testing.T, tcs *TestCaseSet) string {
Copy link
Owner

Choose a reason for hiding this comment

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

Rename to marshalTestCaseSet()?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

Lucas Bremgartner added 2 commits February 6, 2017 10:42
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.
@breml
Copy link
Collaborator Author

breml commented Feb 6, 2017

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
Copy link
Owner

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
Copy link
Collaborator Author

breml commented Feb 9, 2017

@magnusbaeck I had an other look at the filter plugins you mentioned in #21 (review) 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
Copy link
Owner

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

@magnusbaeck
Copy link
Owner

Branch rebased and merged to master.

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.

3 participants