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

[CI/CD] Some integration tests are currently being skipped due to naming restriction #32207

Closed
crobert-1 opened this issue Apr 5, 2024 · 1 comment · Fixed by #32529
Closed
Labels
bug Something isn't working ci-cd CI, CD, testing, build issues test coverage Improve test coverage

Comments

@crobert-1
Copy link
Member

crobert-1 commented Apr 5, 2024

Component(s)

No response

Background

While investigating #32206 I found that running a basic integration test command was causing test failures in the receiver/mongodbatlasreceiver package:

go test -tags=integration

The failing tests (here's one for example) were referencing files whose names changed over a year ago, so I investigated a bit and found that these tests weren't running.

Recent successful CI run output:

Running target 'mod-integration-test' in module 'receiver/mongodbatlasreceiver' as part of group 'receiver-1'
make --no-print-directory -C receiver/mongodbatlasreceiver mod-integration-test
running go integration test ./... in /home/runner/work/opentelemetry-collector-contrib/opentelemetry-collector-contrib/receiver/mongodbatlasreceiver
/home/runner/work/opentelemetry-collector-contrib/opentelemetry-collector-contrib/.tools/gotestsum --rerun-fails=1 --packages="./..." -- -race -timeout 360s -parallel 4 -tags=integration,"" -run=Integration
∅  internal
∅  internal/model
∅  internal/metadata (1.018s)
✓  . (2.101s)

DONE 1 tests in 6.305s

However, the MongoDB Atlas receiver has 4 integration tests, one found here, and three found here.

Problem

I found that the reason not all integration tests are running is because of the -run=Integration option in the following makefile variable:

GOTEST_OPT_WITH_INTEGRATION=$(GOTEST_INTEGRATION_OPT) -tags=integration,$(GO_BUILD_TAGS) -run=Integration

This was introduced here.

From Go's documentation this means that only tests containing Integration in their name are being run.

Impact

I used a script to scan through all of the integration test files and their tests names, only returning tests without Integration in their name. Here are the results:

./extension/healthcheckextension/integration_test.go
func Test_SimpleHealthCheck(t *testing.T) {

./testbed/tests/syslog_integration_test.go
func TestSyslogComplementaryRFC5424(t *testing.T) {
func TestSyslogComplementaryRFC3164(t *testing.T) {

./receiver/hostmetricsreceiver/integration_test.go
func Test_ProcessScrape(t *testing.T) {
func Test_ProcessScrapeWithCustomRootPath(t *testing.T) {
func Test_ProcessScrapeWithBadRootPathAndEnvVar(t *testing.T) {

./receiver/cloudflarereceiver/logs_integration_test.go
func TestReceiverTLS(t *testing.T) {

./receiver/mongodbatlasreceiver/alerts_integration_test.go
func TestAlertsReceiver(t *testing.T) {
func TestAlertsReceiverTLS(t *testing.T) {
func TestAtlasPoll(t *testing.T) {

./exporter/splunkhecexporter/integration_test.go
func TestSplunkHecExporter(t *testing.T) {

./exporter/opensearchexporter/integration_test.go
func TestOpenSearchTraceExporter(t *testing.T) {
func TestOpenSearchLogExporter(t *testing.T) {

./pkg/stanza/adapter/integration_test.go
func TestEmitterToConsumer(t *testing.T) {

Proposed solution

I think the main two options are:

  1. Remove the -run=Integration option so that all tests in a file with the integration build tag are run.
  2. Rename all integration tests to have Integration in their name.

I think 1 is the best option here, as it would be too easy to forget and miss that this is a requirement of integration tests, shown by how long this has been missed.

Side note: Removing the -run argument may have some side effects on running tests that are currently passing. I'm seeing there are four integration tests in the Clickhouse Exporter that are all running successfully right now, but without the argument they hit a panic. There might be some kind of parallelization impact here that may need investigated.

@crobert-1
Copy link
Member Author

Removing needs triage based on PR being approved by project maintainers.

@crobert-1 crobert-1 removed the needs triage New item requiring triage label Apr 25, 2024
codeboten added a commit that referenced this issue May 2, 2024
Including the `-run=Integration` argument in the `make` command means
that only integration tests that include `Integration` in their name
will be run. This requirement is not obvious to users, so there are
currently around 15 integration tests that aren't being run because they
don't conform to this requirement. I think the best approach here is to
remove the `-run` argument (rather than rename tests) so that this
doesn't happen again.

Resolves
#32207

---------

Co-authored-by: Antoine Toulme <antoine@toulme.name>
Co-authored-by: Tyler Helmuth <12352919+TylerHelmuth@users.noreply.github.com>
Co-authored-by: Alex Boten <223565+codeboten@users.noreply.github.com>
rimitchell pushed a commit to rimitchell/opentelemetry-collector-contrib that referenced this issue May 8, 2024
…etry#32529)

Including the `-run=Integration` argument in the `make` command means
that only integration tests that include `Integration` in their name
will be run. This requirement is not obvious to users, so there are
currently around 15 integration tests that aren't being run because they
don't conform to this requirement. I think the best approach here is to
remove the `-run` argument (rather than rename tests) so that this
doesn't happen again.

Resolves
open-telemetry#32207

---------

Co-authored-by: Antoine Toulme <antoine@toulme.name>
Co-authored-by: Tyler Helmuth <12352919+TylerHelmuth@users.noreply.github.com>
Co-authored-by: Alex Boten <223565+codeboten@users.noreply.github.com>
jlg-io pushed a commit to jlg-io/opentelemetry-collector-contrib that referenced this issue May 14, 2024
…etry#32529)

Including the `-run=Integration` argument in the `make` command means
that only integration tests that include `Integration` in their name
will be run. This requirement is not obvious to users, so there are
currently around 15 integration tests that aren't being run because they
don't conform to this requirement. I think the best approach here is to
remove the `-run` argument (rather than rename tests) so that this
doesn't happen again.

Resolves
open-telemetry#32207

---------

Co-authored-by: Antoine Toulme <antoine@toulme.name>
Co-authored-by: Tyler Helmuth <12352919+TylerHelmuth@users.noreply.github.com>
Co-authored-by: Alex Boten <223565+codeboten@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working ci-cd CI, CD, testing, build issues test coverage Improve test coverage
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant