-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
[WIP/Ideas] Config matrices for integration tests #7957
Conversation
libbeat/tests/compose/runner.go
Outdated
return scenarios | ||
} | ||
|
||
func (r *TestRunner) Run(t *testing.T, tests ...func(t *testing.T, host string)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
exported method TestRunner.Run should have comment or be unexported
"testing" | ||
) | ||
|
||
type TestRunner struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
exported type TestRunner should have comment or be unexported
compose.EnsureUp(t, "redis") | ||
|
||
addEntry(t) | ||
runner.Run(t, func(t *testing.T, host string) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice idea 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am starting to think in the merge of this little monster and one of my main concerns is that this tests structure modifies the whole integration test files even if only for the indentation level. This is already making the maintenance of the branch a bit painful, as any change on integration tests require a manual merge when updating the branch. And for the same reasons it will be also painful when merged for open PRs and backports.
I am thinking on a second option, in which there is a simple test that calls the runner and the actual tests are defined as private functions. Find in this gist both options for a simple test file and their diffs with master. Option 1 is the one I'm following by now, option 2 would be my new proposal, in option 2 the differences are actually only in the lines that would need to be changed, and there are less indentation levels.
@andrewkroh @ruflin opinions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
any disadvantages for option 2 besides having more functions? +1 on 2 so far.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The more I think about it, the more I am leaning about second option too. One disadvantage can be that one needs to add the test function to the suite for it to be run, but most of integration tests have only a fetch and a data function and is not so frequent to add more tests.
I'll go for option 2.
jenkins, test this |
"github.com/pkg/errors" | ||
) | ||
|
||
type TestRunner struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
exported type TestRunner should have comment or be unexported
) | ||
|
||
var ( | ||
Runner = compose.TestRunner{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
exported var Runner should have comment or be unexported
libbeat/tests/compose/runner.go
Outdated
Timeout int | ||
} | ||
|
||
type Suite map[string]func(t *testing.T, host string) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
exported type Suite should have comment or be unexported
rand.Seed(time.Now().UnixNano()) | ||
} | ||
|
||
type TestRunner struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
exported type TestRunner should have comment or be unexported
e609b43
to
c463a75
Compare
c463a75
to
0f1a8d6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
++ on having this.
DataRunner = compose.TestRunner{ | ||
Service: "redis", | ||
Options: map[string][]string{ | ||
"REDIS_VERSION": []string{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 on always having the newest version here which is GA.
var ( | ||
Runner = compose.TestRunner{ | ||
Service: "redis", | ||
Options: map[string][]string{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great. Perhaps in the future this could even be generated from the fields.yml
file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I want to keep this as a struct with basic data in case we move it to a yaml file.
libbeat/tests/compose/runner.go
Outdated
timeout = 300 | ||
} | ||
|
||
for _, s := range r.scenarios() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be nice to still have something like NO_COMPOSE=1
that can be set if the environment is already running on the local machine so it will not start up all docker containers. Perhaps that still exists ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have just added something like this, look at 89f5f0e
I prefer to do it as a host override instead of only a feature disable for two reasons:
- Depending on how you are running the environment you want to test with you are going to need to override the host in any case
- "Running environment" is going to be trickier now, as each service can be running with multiple versions and configurations
Add the possibility to override some options of compose runner using environment variables: - If there is a variable with the upercased name of the service + "_HOST" (i.e. REDIS_HOST for "redis" service), no scenario is created and the value of the variable is injected to the test in the host variable. - If there is a variable named as one of the options, this single value is used instead of the list of values in the options matrix.
var offsetBefore int64 = 0 | ||
var offsetAfter int64 = 0 | ||
var offsetBefore int64 = 0 | ||
var offsetAfter int64 = 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should drop = 0 from declaration of var offsetAfter; it is the zero value
|
||
var offsetBefore int64 = 0 | ||
var offsetAfter int64 = 0 | ||
var offsetBefore int64 = 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should drop = 0 from declaration of var offsetBefore; it is the zero value
7f274cf
to
ffe2062
Compare
dsn = "http://Administrator:password@localhost:8091" | ||
} | ||
return dsn | ||
func GetEnvDSN(host string) string { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
exported function GetEnvDSN should have comment or be unexported
// Runner is the compose test runner for aerospike | ||
Runner = compose.TestRunner{ | ||
Service: "aerospike", | ||
Options: compose.RunnerOptions{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The config files I started to introduce here could be a place where we have this documented in the future: https://github.com/elastic/beats/pull/11131/files#diff-7d1232f25fa4ea3006884196c00228c7R3 (not for this PR)
Add a helper to automatically obtain the host of a service in metricbeat integration tests so tests don't depend on environment variables or current running docker services. This is needed to allow in the future to run multiple versions of the same service, is also helpful when running tests locally. Environment variables with the format SERVICE_HOST, being SERVICE the uppercased name of the compose service can be used to override the automatic value. `NO_COMPOSE` environment variable can be used now to avoid stopping the running containers too. Add ports to all services in docker compose files, required for the helper. Part of elastic#7957 for python-based integration tests.
Add a helper to automatically obtain the host of a service in metricbeat integration tests so tests don't depend on environment variables or current running docker services. This is needed to allow in the future to run multiple versions of the same service, is also helpful when running tests locally. Environment variables with the format `SERVICE_HOST`, being SERVICE the uppercased name of the compose service can be used to override the automatic value. `NO_COMPOSE` environment variable can be used now to avoid stopping the running containers too. Add ports to all services in docker compose files, required for the helper. Part of #7957 for python-based integration tests.
Closing this as it is not going to be merged as is, and many of the ideas explored here have been already merged. Also another approach for end to end testing that could cover testing with multiple versions is being explored in https://github.com/elastic/metricbeat-tests-poc |
Thoughts and experiments for having the possibility of integration tests with more configurations in an easier way, and to continue migrating things from python to go. Not to be merged yet (even if miraculously tests pass 😃 )
Here one metricset of the redis module is tested with three versions and two operating systems. It also makes better use of paralellization. Runners could be easily shared per module.
Some things done here:
docker-compose down -v
)