Re-enabling golang integration tests #15819

ycombinator · 2020-01-24T13:31:21Z

Somewhere along the way we inadvertently stopped runing golang integration tests. This PR re-enables them.

ycombinator · 2020-01-24T20:50:24Z

libbeat/scripts/Makefile

+	  -e RACE_DETECTOR=$(RACE_DETECTOR) \
+	  -e DOCKER_COMPOSE_PROJECT_NAME=${DOCKER_COMPOSE_PROJECT_NAME} \
+	  -e TEST_ENVIRONMENT=${TEST_ENVIRONMENT} \
+	  -e BEATS_DOCKER_INTEGRATION_TEST_ENV=1 \


This line is what makes the integration tests run again in CI. But I'm not 100% sure if this is the right fix or the right place to make this fix. Any thoughts @urso @mtojek?

I can confirm that I focused mostly on the continuously failing integration tests for x-pack/metricbeat and assumed that the execution path at this level is same for both. Sorry about that.

I see test output in the Travis CI log proving it's working now. I don't know about any requirements that prevent you from merging this change. Thanks!

side note:
It would be useful if these magic envs are somewhere documented (what are they for, why not autodiscovered): BEATS_DOCKER_INTEGRATION_TEST_ENV, TEST_ENVIRONMENT, etc.

ycombinator · 2020-01-24T21:59:32Z

metricbeat/module/windows/perfmon/perfmon_test.go

@@ -15,7 +15,8 @@
 // specific language governing permissions and limitations
 // under the License.

-// +build integration windows
+// +build integration
+// +build windows


Without this change this integration test was being run on Linux. Multiple build tags on the same line are combined with a logical OR whereas multiple build tags on different lines are combined with an "AND". We want the latter effect for this test.

mtojek · 2020-01-27T07:06:31Z

libbeat/scripts/Makefile

+	  -e RACE_DETECTOR=$(RACE_DETECTOR) \
+	  -e DOCKER_COMPOSE_PROJECT_NAME=${DOCKER_COMPOSE_PROJECT_NAME} \
+	  -e TEST_ENVIRONMENT=${TEST_ENVIRONMENT} \
+	  -e BEATS_DOCKER_INTEGRATION_TEST_ENV=1 \


I can confirm that I focused mostly on the continuously failing integration tests for x-pack/metricbeat and assumed that the execution path at this level is same for both. Sorry about that.

I see test output in the Travis CI log proving it's working now. I don't know about any requirements that prevent you from merging this change. Thanks!

side note:
It would be useful if these magic envs are somewhere documented (what are they for, why not autodiscovered): BEATS_DOCKER_INTEGRATION_TEST_ENV, TEST_ENVIRONMENT, etc.

urso · 2020-01-27T12:14:31Z

@andrewkroh Can you please have a look if the use of the environment variables is correct?

Are we missing environment variables in the Makefile? I just found the variable + documentation in some go file.

What is the reason to run go based integration tests from within a docker container? When running tests from within a container we only run integration tests on Linux. It looks like on windows/macOS we use a very different code path (via make testsuite) to run the integration tests. Is there a chance to reduce complexity by consolidating the way we run the integration tests on all platforms?

andrewkroh · 2020-01-28T16:38:11Z

BEATS_DOCKER_INTEGRATION_TEST_ENV is used exclusively by Mage when running integration tests. It uses this environment variable to determine if it is running inside the containerized environment that it started (so that it doesn't try to start the containers again).

This value should not need to be set anywhere other than where mage sets it automatically. So I suspect something else changed with how the tests are invoked...

I see that libbeat/scripts/Makefile has been updated to invoke mage goIntegTest. This causes a problem because both the Makefile and Mage want to manage the setup of a Dockerized test environment.

The fix here to add BEATS_DOCKER_INTEGRATION_TEST_ENV to the environment created by the Makefile will probably work. But rather than trying to change libbeat/scripts/Makefile to invoke mage targets I'd rather see any Beat that has implemented testing with mage move away from using libbeat/scripts/Makefile by following what x-pack/metricbeat does. x-pack/metricbeat just uses small Makefile shim to implement the targets need by Jenkins.

ycombinator · 2020-01-28T16:39:31Z

Thanks for the clear explanation @andrewkroh. I'll implement the shim in the OSS Metricbeat Makefile as you suggested.

ycombinator · 2020-01-28T20:26:12Z

@andrewkroh @urso @mtojek I looked at what x-pack/metricbeat does and it is very clean indeed. I think it's a good idea to clean up the OSS Metricbeat Makefile similarly but I'd prefer to defer that cleanup to a later PR. I'd like this PR here to focus on getting OSS Metricbeat integration tests running again in CI since they haven't been run in 3+ weeks.

I did take a couple of suggestions from @andrewkroh's comment, viz.:

instead of invoking mage from the libbeat Makefile's integration-tests, I am now overriding this target in the OSS Metricbeat Makefile.
instead of hardcoding the BEATS_DOCKER_INTEGRATION_TEST_ENV variable in the libbeat Makefile's integration-tests-environment target command, I am now setting this environment variable at the top of the OSS Metricbeat Makefile.

This way, at least most of the changes are now scoped to the OSS Metricbeat Makefile.

Please let me know what you think. Thanks!

andrewkroh

LGTM for getting the tests running again. We can revisit the converting the project over to a pure Go / mage based build in a few weeks.

ycombinator · 2020-01-29T15:28:20Z

Thanks @andrewkroh.

Related: #9842.

* Adding debugging * Still debugging * Add deps for integration tests * Making progress, I think :) * Testing * Try a different way of passing the env var * Revert docker info change * Removing debugging statements * Ensure "AND"ing of build tags * Don't explicitly pass BEATS_DOCKER_INTEGRATION_TEST_ENV * Move mage goIntegTest usage to metricbeat makefile * Fixing up comment * Forgot the line continuation mark * Pass the env var

Adding debugging

6286301

ycombinator added the in progress Pull request is currently in progress. label Jan 24, 2020

ycombinator changed the title ~~Re-enabling Metricbeat golang integration tests~~ Re-enabling golang integration tests Jan 24, 2020

ycombinator added 7 commits January 24, 2020 06:10

Still debugging

4c0e33e

Add deps for integration tests

0f2af2c

Making progress, I think :)

a01beab

Testing

cb2ed6e

Try a different way of passing the env var

23bff34

Revert docker info change

efa0c46

Removing debugging statements

9ac6477

ycombinator commented Jan 24, 2020

View reviewed changes

ycombinator requested review from urso, 1stvamp and mtojek and removed request for 1stvamp January 24, 2020 20:50

ycombinator added :Testing review Team:Beats needs_backport PR is waiting to be backported to other branches. v7.7.0 v8.0.0 and removed in progress Pull request is currently in progress. labels Jan 24, 2020

Ensure "AND"ing of build tags

1e9c7c6

ycombinator requested a review from a team as a code owner January 24, 2020 21:10

ycombinator commented Jan 24, 2020

View reviewed changes

ycombinator added the v7.6.1 label Jan 24, 2020

mtojek approved these changes Jan 27, 2020

View reviewed changes

ycombinator mentioned this pull request Jan 27, 2020

Adding integration test for logstash Metricbeat module, xpack code path #15800

Merged

urso requested a review from andrewkroh January 27, 2020 12:08

ycombinator added 5 commits January 28, 2020 10:20

Don't explicitly pass BEATS_DOCKER_INTEGRATION_TEST_ENV

4375bb3

Move mage goIntegTest usage to metricbeat makefile

bef6960

Fixing up comment

7aaaeec

Forgot the line continuation mark

d530cb3

Pass the env var

bef4f89

andrewkroh approved these changes Jan 29, 2020

View reviewed changes

ycombinator merged commit a80a914 into elastic:master Jan 29, 2020

ycombinator deleted the go-integ-pass-env-vars branch January 29, 2020 15:28

This was referenced Jan 29, 2020

[7.x] Re-enabling golang integration tests (#15819) #15933

Merged

[7.6] Re-enabling golang integration tests (#15819) #15934

Merged

andresrc added the [zube]: Done label Jan 30, 2020

ycombinator removed the needs_backport PR is waiting to be backported to other branches. label Jan 30, 2020

andresrc removed the [zube]: Done label Feb 4, 2020

andresrc added the Team:Integrations Label for the Integrations team label Mar 6, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Re-enabling golang integration tests #15819

Re-enabling golang integration tests #15819

ycombinator commented Jan 24, 2020 •

edited

Loading

ycombinator Jan 24, 2020 •

edited

Loading

mtojek Jan 27, 2020

ycombinator Jan 24, 2020 •

edited

Loading

mtojek Jan 27, 2020

urso commented Jan 27, 2020

andrewkroh commented Jan 28, 2020

ycombinator commented Jan 28, 2020

ycombinator commented Jan 28, 2020 •

edited

Loading

andrewkroh left a comment

ycombinator commented Jan 29, 2020

Re-enabling golang integration tests #15819

Re-enabling golang integration tests #15819

Conversation

ycombinator commented Jan 24, 2020 • edited Loading

ycombinator Jan 24, 2020 • edited Loading

Choose a reason for hiding this comment

mtojek Jan 27, 2020

Choose a reason for hiding this comment

ycombinator Jan 24, 2020 • edited Loading

Choose a reason for hiding this comment

mtojek Jan 27, 2020

Choose a reason for hiding this comment

urso commented Jan 27, 2020

andrewkroh commented Jan 28, 2020

ycombinator commented Jan 28, 2020

ycombinator commented Jan 28, 2020 • edited Loading

andrewkroh left a comment

Choose a reason for hiding this comment

ycombinator commented Jan 29, 2020

ycombinator commented Jan 24, 2020 •

edited

Loading

ycombinator Jan 24, 2020 •

edited

Loading

ycombinator Jan 24, 2020 •

edited

Loading

ycombinator commented Jan 28, 2020 •

edited

Loading