Support multiple system tests per data_stream #209

adriansr · 2020-12-21T17:44:34Z

Updates the system tests runner so that multiple test-*-config.yml files can be included in <data_stream>/_dev/tests/system. This allows testing multiple inputs and/or different configuration options per data stream.

Example test for two inputs:

test-tcp-input.yml:

  input: tcp
  service: bluecoat-director-tcp
  vars: ~
  data_stream:
    vars:
      tcp_host: 0.0.0.0
      tcp_port: 9514

test-udp-input.yml:

  input: udp
  service: bluecoat-director-udp
  vars: ~
  data_stream:
    vars:
      udp_host: 0.0.0.0
      udp_port: 9514

The service tags allows to uses different containers for the tests. In this case, to generate traffic using different protocols.

Sample docker-compose.yml in <package>/_dev/deploy/docker:

version: '2.3'
services:
  bluecoat-director-tcp:
    tty: true
    build: .
    volumes:
      - ./logs:/logs/:ro
    command: /stream -proto tcp -dest elastic-agent:9514 /logs/generated.log
  bluecoat-director-udp:
    tty: true
    build: .
    volumes:
      - ./logs:/logs/:ro
    command: /stream -proto udp -delay 100ms -dest elastic-agent:9514 /logs/generated.log

Related #208
Related elastic/package-spec#101

elasticmachine · 2020-12-21T17:47:29Z

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS

Expand to view the summary

Build stats

Build Cause: Pull request #209 updated
Start Time: 2021-01-12T15:22:18.551+0000
Duration: 11 min 42 sec

Test stats 🧪

Test	Results
Failed	0
Passed	11
Skipped	0
Total	11

adriansr · 2020-12-22T15:54:59Z

/test

ycombinator · 2020-12-28T14:21:48Z

This is a great addition, @adriansr. I was wondering how long before we needed it 🙂 .

I was on PTO all last week so I'm only seeing this PR now. I should be able to review it today or tomorrow at the latest.

ycombinator · 2020-12-29T17:45:46Z

internal/testrunner/runners/system/runner.go

+	// Determine test name
+	name := config.Name
+	if name == "" {
+		name = config.Input


I understand that the use case for this change is to test with different inputs but I think your solution is more generic than that. So I would not fallback to config.Input if config.Name is not defined. I think we can require developers to specify config.Name always, so if it's empty I think we should error out.

ycombinator · 2020-12-29T17:51:12Z

internal/testrunner/runners/system/test_config.go

+		}
+		c = append(c, single)
+	}
+	return c, nil


This is nice but I'm wondering if we should simplify and require that the test config should always be a list of cases. That way it's also a hint to package developers that multiple test cases are allowed. Of course, this would mean updating existing system test configurations.

I think this PR has been started too early. It's nice to have a PoC showing the concept, but ideally we kick off with a proposal in package-spec, as it should be adjusted first.

What do you think about keeping the same order as in pipeline tests, which support multiple test cases per data_stream? See: https://github.com/elastic/integrations/tree/master/packages/apache/data_stream/access/_dev/test/pipeline
I'm referring to the name pattern: test-name.config.yml.

Regarding the config.yml, I think we have to adjust all integrations, but as there is dependency versioning in place, I don't think we need to rollout changes synchronically.

IMHO the order of steps is the following:

PR to the package-spec introducing new config.yml.

This PR with updated package-spec dependency.

PR to the Integrations repository polishing all config.ymls

ycombinator

Just left a couple of minor comments.

mtojek

left a general comment

The <data_stream>/_dev/tests/system/config.yml can now be a list of tests, so that multiple inputs and / or multiple configurations can be tested.

Some tests configuration needs variables from the docker-compose, so it's necessary to regenerate the test config.yml after starting the project.

mtojek

@adriansr I'm afraid that changes you pushed to the package-spec blocked the other team (#216) and will be a problem in Integrations as the elastic-package with new spec won't accept any existing config.yml files.

I'm afraid we need to solve it ASAP.

adriansr · 2021-01-12T12:15:49Z

@mtojek this PR is ready for review and incorporates the changes from #216 (because it also needs to update the package-spec dependency).

mtojek · 2021-01-12T12:37:01Z

I see it's ready for review, but once you push this, you will also block updating the elastic-package in Integrations without adjusting multiple files. I'm not convinced about the timeline and wouldn't like to block people. Do you plan to adjust all integrations immediately?

I'm referring to this line:

var systemTestConfigFilePattern = regexp.MustCompile(`^test-([a-z0-9_.-]+)-config.yml$`)

ycombinator · 2021-01-12T12:52:11Z

internal/testrunner/runners/system/test_config.go


 type testConfig struct {
 	Input      string                       `config:"input"`
+	Service    string                       `config:"service"`


@mtojek This is more of a note for us for the future: as we add more service deployers we will need to make sure this field continues to work as expected with the new service deployers as well (not just the Docker compose one that we have today).

ycombinator · 2021-01-12T12:55:10Z

test/packages/multiinput/data_stream/test/agent/stream/tcp.yml.hbs

@@ -0,0 +1,22 @@
+tcp:
+host: "{{tcp_host}}:{{tcp_port}}"


Is the indentation here correct?

Turns out is not an indentation problem. That tcp key is just meaningless. Seems like a few packages include this mistake and we've just been repeating it in some places.

Okay, thanks. Can you make an issue (or PR, whatever is easier), probably in the integrations repo to clean up the meaningless keys?

test/packages/multiinput/data_stream/test/agent/stream/udp.yml.hbs

ycombinator

Left a couple of minor comments/questions on indentation. Besides those, this PR LGTM.

Would be nice to have a sister PR ready to go in https://github.com/elastic/integrations that updates all existing system test configs per the new naming convention and syntax introduced in this PR. That way you can just update that PR with the elastic-package dependency once this PR here is merged and we can quickly get all integrations updated as well.

Updates the system tests for the packages that use them to conform to the new format introduced in elastic/package-spec#101 and incorporated in elastic-package tool in elastic/elastic-package#209. This also disables 3 tests that are failing: - cef - crowdstrike - nats/route

This reverts part of the changes in #209 as some tests may require more than one container running at the same time.

Updates the system tests for the packages that use them to conform to the new format introduced in elastic/package-spec#101 and incorporated in elastic-package tool in elastic/elastic-package#209. This also disables 3 tests that are failing: - cef - crowdstrike - nats/route

adriansr marked this pull request as draft December 21, 2020 19:51

marc-gr mentioned this pull request Dec 22, 2020

Add sophos xg integration elastic/integrations#479

Merged

7 tasks

adriansr marked this pull request as ready for review December 22, 2020 15:49

adriansr added the enhancement New feature or request label Dec 22, 2020

adriansr requested review from ycombinator, andrewkroh and mtojek December 23, 2020 10:03

ycombinator reviewed Dec 29, 2020

View reviewed changes

ycombinator requested changes Dec 29, 2020

View reviewed changes

mtojek reviewed Jan 4, 2021

View reviewed changes

jonathan-buttner mentioned this pull request Jan 11, 2021

Update package_spec #216

Closed

adriansr added 9 commits January 12, 2021 08:24

Support multiple system tests per data_stream

514d394

The <data_stream>/_dev/tests/system/config.yml can now be a list of tests, so that multiple inputs and / or multiple configurations can be tested.

Fix some ordering and comments

345b1a8

Fix initialization of agent folders

4146704

Re-render configuration after service up

95f4685

Some tests configuration needs variables from the docker-compose, so it's necessary to regenerate the test config.yml after starting the project.

Add sample multi-input package for system tests

ccdad41

Missing license

7fdb3e9

Remove unnecessary fields file

58a001e

Split system test case configuration into multiple files

6c5ebbb

Update package-spec dependency

60ee5a8

mtojek self-requested a review January 12, 2021 09:28

mtojek suggested changes Jan 12, 2021

View reviewed changes

ycombinator self-requested a review January 12, 2021 12:47

ycombinator reviewed Jan 12, 2021

View reviewed changes

mtojek self-requested a review January 12, 2021 12:54

ycombinator reviewed Jan 12, 2021

View reviewed changes

test/packages/multiinput/data_stream/test/agent/stream/udp.yml.hbs Show resolved Hide resolved

ycombinator approved these changes Jan 12, 2021

View reviewed changes

adriansr mentioned this pull request Jan 12, 2021

Update system tests to multi-case format elastic/integrations#514

Merged

5 tasks

Remove unnecessary tags in testing package

d83e438

mtojek approved these changes Jan 12, 2021

View reviewed changes

adriansr merged commit 783d466 into elastic:master Jan 12, 2021

ChrsMark mentioned this pull request Jan 13, 2021

Ensure all composed services are up for nats cluster testing elastic/integrations#525

Merged

adriansr mentioned this pull request Jan 13, 2021

System tests: start all services in compose project #220

Merged

mtojek pushed a commit that referenced this pull request Jan 14, 2021

System tests: start all services in compose project (#220)

743ad7d

This reverts part of the changes in #209 as some tests may require more than one container running at the same time.

andrewkroh mentioned this pull request Feb 11, 2021

[System test runner] Support for multiple tests per data stream #208

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support multiple system tests per data_stream #209

Support multiple system tests per data_stream #209

adriansr commented Dec 21, 2020 •

edited

Loading

elasticmachine commented Dec 21, 2020 •

edited

Loading

Build stats

Test stats 🧪

adriansr commented Dec 22, 2020

ycombinator commented Dec 28, 2020 •

edited

Loading

ycombinator Dec 29, 2020

ycombinator Dec 29, 2020 •

edited

Loading

mtojek Jan 4, 2021

ycombinator left a comment

mtojek left a comment

mtojek left a comment

adriansr commented Jan 12, 2021

mtojek commented Jan 12, 2021

ycombinator Jan 12, 2021

ycombinator Jan 12, 2021

adriansr Jan 12, 2021

ycombinator Jan 12, 2021

ycombinator left a comment

Support multiple system tests per data_stream #209

Support multiple system tests per data_stream #209

Conversation

adriansr commented Dec 21, 2020 • edited Loading

elasticmachine commented Dec 21, 2020 • edited Loading

💚 Build Succeeded

Build stats

Test stats 🧪

adriansr commented Dec 22, 2020

ycombinator commented Dec 28, 2020 • edited Loading

ycombinator Dec 29, 2020

Choose a reason for hiding this comment

ycombinator Dec 29, 2020 • edited Loading

Choose a reason for hiding this comment

mtojek Jan 4, 2021

Choose a reason for hiding this comment

ycombinator left a comment

Choose a reason for hiding this comment

mtojek left a comment

Choose a reason for hiding this comment

mtojek left a comment

Choose a reason for hiding this comment

adriansr commented Jan 12, 2021

mtojek commented Jan 12, 2021

ycombinator Jan 12, 2021

Choose a reason for hiding this comment

ycombinator Jan 12, 2021

Choose a reason for hiding this comment

adriansr Jan 12, 2021

Choose a reason for hiding this comment

ycombinator Jan 12, 2021

Choose a reason for hiding this comment

ycombinator left a comment

Choose a reason for hiding this comment

adriansr commented Dec 21, 2020 •

edited

Loading

elasticmachine commented Dec 21, 2020 •

edited

Loading

ycombinator commented Dec 28, 2020 •

edited

Loading

ycombinator Dec 29, 2020 •

edited

Loading