Get TestData back into the metricsets

[jsoriano]

Add helpers to the recently added testdata framework so modules can still
be tested with go test ./metricbeat/module/foo/....

It keeps by now the TestAll test, we can revisit this decission in the future,
while this is kept these tests will be run twice on CI.

[sayden]

Thanks for contributing with this! Left a couple of comments

[ruflin]

What bugs me about the current approach we have is that go test . inside a metricset does not work. This is solved by this PR. But there are 2 downsides here:

• It introduces additional boilerplate code (I could leave with that)
• In case someone adds the testsdata but forgets to add the TestData function, tests are not run.

It's easy to miss the second point in a review. It means all data could have been added but is not run properly and we will not detect it.

I wonder if there is any other trick that we can make go test . working but have TestAll still around? I'm not even worried if things are executed twice as it should be super quick to execute. I rather pay the double execution than missing some tests.

[exekias]

Having go test . in the module folder is great, it makes modules development standalone, no need to exit their folder.

I agree with the second point, TestAll could be mimicked with this (maybe, I believe?):

• Remove all expected files
• Run tests with generate flag
• git diff should return nothing

[jsoriano]

* It introduces additional boilerplate code (I could leave with that)

I am trying it to be the minimal needed boilerplate, but I think that an explicit test is required, and even desired.

* In case someone adds the testsdata but forgets to add the `TestData` function, tests are not run.

It's easy to miss the second point in a review. It means all data could have been added but is not run properly and we will not detect it.

I think we can live with this risk 🙂 Something similar would happen with any other test that uses external files, one may add the files but forget the test.
In this case the risk is mitigated because the framework is also helpful in generating the expected outputs and the data.json file, so it will be missed if forgotten 🙂

Another thing that mitigates the risk is that this way all metricsets continue having tests, it is easier for newcomers to understand how the module is being tested, they don't need to guess that there are some testdata files in _meta that are somehow used for tests. They can see a test, that calls the helpers, and then look at the docs (or the code) of the helpers.

Relying on conventions is also prone to add things that are not interpreted as expected or ignored by the framework.

I wonder if there is any other trick that we can make go test . working but have TestAll still around? I'm not even worried if things are executed twice as it should be super quick to execute. I rather pay the double execution than missing some tests.

I'd like to end-up removing TestAll so we don't continue relying on it, I'd like modules and metricsets to contain their own tests.
We could use general coverage-based solutions to automatically check if a metricset is being tested or not, I'd prefer to base this on something that can be useful for all the code.

For initial migration, and sporadic checks we can follow @exekias recommendation of removing all expected files and run the tests with the generate flag.

[ruflin]

One thought I would like to add here: What happens in the future if we have modules without go code?

I think if we keep the TestAll around combined with this change, we have the best of both worlds? Any downside to this expect tests running twice?

[jsoriano]

I'd still place the tests for these modules near these modules (we can have go test files in packages without other test code). But ok, we can delay this discussion, I will leave TestAll by now.

[jsoriano]

I added back the test methods but keeping TestAll too, I tried to regenerate all expected.json file and it worked as expected.

The only problem with go test ./metricset/module/... -generate is that modules without this flag give an error, I guess this is not such a problem as this flag will be mostly used when working on metricsets.

[jsoriano]

This is the only thing that changed after regenerating all files.

[ruflin]

I'm surprised this changed (it shouldn't). @sayden I remember we had some discussion around apache and "whitelist" some files which should not be remove or similar?

[sayden]

Yes, I finally added a config in TestAll to omit specific fields or fields group from the comparison because they were changing in each regenerate https://github.com/elastic/beats/pull/11714/files#diff-5b51ecdcb61bd7977eb4d2e2852178c0R32

[ruflin]

And why did it still change here?

[ruflin]

@sayden ^

[ruflin]

We don't need this anymore?

[jsoriano]

I couldn't find where this was being used, so I removed it.

Could this be the cause of the change in apache hostname?

[sayden]

No. It was there as reference. Please, undo 🙂

[sayden]

Apache hostname is changing because it takes the hostname of the beat and writes it in each execution at the metricset level. We discussed about leaving it like this for compatibility reasons. Here you can see that m.Host is passed to the eventMapping which sets it here

[jsoriano]

Wdyt about adding it as example in the godocs? If my future self finds this file alone here he will probably wonder again what it is.

[jsoriano]

Apache hostname is changing because it takes the hostname of the beat and writes it in each execution at the metricset level. We discussed about leaving it like this for compatibility reasons. Here you can see that m.Host is passed to the eventMapping which sets it here

Ok, I see it also changes when running TestAll with generate, I won't change this in this PR.

[ruflin]

Why does the module need to be included? Does otherwise some functionality miss?

[jsoriano]

This is needed for the fields checks, without this the module fields are not included and then the check fails. The TestAll version includes all modules when including github.com/elastic/beats/metricbeat/include.

I was tempted of just moving this include to mb/testing, but this way we check that each module defines its own fields.

Common fields are included from mb/testing.

Not sure if there is other way of including the module fields only.

[ruflin]

LGTM

I like the hybrid we have. I can see potential that in some cases the local tests will break because of some ECS fields needed which then are not there. But lets cross this bridge when we get there.

[exekias]

Let's see if tests pass 🙏