From bb7f2f15b3205e2581810e164470fbc84612e819 Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Mon, 28 Feb 2022 16:13:53 -0500 Subject: [PATCH 1/2] ca: require that tests that use Vault are named correctly Previously we were using two different criteria to decide where to run a test. The main `go-test` job would skip Vault tests based on the presence of the `vault` binary, but the `test-connect-ca-providers` job would run tests based on the name. This led to a scenario where a test may never run in CI. To fix this problem I added a name check to the function we use to skip the test. This should ensure that any test that requires vault is named correctly to be run as part of the `test-connect-ca-providers` job. At the same time I relaxed the regex we use. I verified this runs the same tests using `go test --list Vault`. I made this change because a bunch of tests in `agent/connect/ca` used `Vault` in the name, without the underscores. Instead of changing a bunch of test names, this seemed easier. With this approach, the worst case is that we run a few extra tests in the `test-connect-ca-providers` job, which doesn't seem like a problem. --- GNUmakefile | 8 ++++---- agent/connect/ca/testing.go | 8 ++++++++ 2 files changed, 12 insertions(+), 4 deletions(-) diff --git a/GNUmakefile b/GNUmakefile index 4742cd878..c24df9308 100644 --- a/GNUmakefile +++ b/GNUmakefile @@ -330,15 +330,15 @@ ifeq ("$(CIRCLECI)","true") # Run in CI gotestsum --format=short-verbose --junitfile "$(TEST_RESULTS_DIR)/gotestsum-report.xml" -- -cover -coverprofile=coverage.txt ./agent/connect/ca # Run leader tests that require Vault - gotestsum --format=short-verbose --junitfile "$(TEST_RESULTS_DIR)/gotestsum-report-leader.xml" -- -cover -coverprofile=coverage-leader.txt -run '.*_Vault_' ./agent/consul + gotestsum --format=short-verbose --junitfile "$(TEST_RESULTS_DIR)/gotestsum-report-leader.xml" -- -cover -coverprofile=coverage-leader.txt -run Vault ./agent/consul # Run agent tests that require Vault - gotestsum --format=short-verbose --junitfile "$(TEST_RESULTS_DIR)/gotestsum-report-agent.xml" -- -cover -coverprofile=coverage-agent.txt -run '.*_Vault_' ./agent + gotestsum --format=short-verbose --junitfile "$(TEST_RESULTS_DIR)/gotestsum-report-agent.xml" -- -cover -coverprofile=coverage-agent.txt -run Vault ./agent else # Run locally @echo "Running /agent/connect/ca tests in verbose mode" @go test -v ./agent/connect/ca - @go test -v ./agent/consul -run '.*_Vault_' - @go test -v ./agent -run '.*_Vault_' + @go test -v ./agent/consul -run Vault + @go test -v ./agent -run Vault endif proto: $(PROTOGOFILES) $(PROTOGOBINFILES) diff --git a/agent/connect/ca/testing.go b/agent/connect/ca/testing.go index 92673dcb9..97c28871d 100644 --- a/agent/connect/ca/testing.go +++ b/agent/connect/ca/testing.go @@ -6,6 +6,7 @@ import ( "io/ioutil" "os" "os/exec" + "strings" "sync" "github.com/hashicorp/go-hclog" @@ -86,6 +87,13 @@ func TestConsulProvider(t testing.T, d ConsulProviderStateDelegate) *ConsulProvi // These tests may be skipped in CI. They are run as part of a separate // integration test suite. func SkipIfVaultNotPresent(t testing.T) { + // Try to safeguard against tests that will never run in CI. + // This substring should match the pattern used by the + // test-connect-ca-providers CI job. + if !strings.Contains(t.Name(), "Vault") { + t.Fatalf("test name must contain Vault, otherwise CI will never run it") + } + vaultBinaryName := os.Getenv("VAULT_BINARY_NAME") if vaultBinaryName == "" { vaultBinaryName = "vault" From dd565aa5e4121f90fd154d9ef86770982bb80685 Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Mon, 28 Feb 2022 16:26:18 -0500 Subject: [PATCH 2/2] ca: fix a test This test does not use Vault, so does not need ca.SkipIfVaultNotPresent --- agent/consul/leader_connect_test.go | 2 -- 1 file changed, 2 deletions(-) diff --git a/agent/consul/leader_connect_test.go b/agent/consul/leader_connect_test.go index 901154eee..bbb9bf432 100644 --- a/agent/consul/leader_connect_test.go +++ b/agent/consul/leader_connect_test.go @@ -1490,8 +1490,6 @@ func TestCAManager_Initialize_Vault_BadCAConfigDoesNotPreventLeaderEstablishment } func TestCAManager_Initialize_BadCAConfigDoesNotPreventLeaderEstablishment(t *testing.T) { - ca.SkipIfVaultNotPresent(t) - _, s1 := testServerWithConfig(t, func(c *Config) { c.Build = "1.9.1" c.PrimaryDatacenter = "dc1"