From c32650e9d8af3e7175aebf176e50da9cb8f63508 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Peter=20=C5=A0tibran=C3=BD?= Date: Tue, 9 Sep 2025 10:16:12 +0200 Subject: [PATCH] Replace remaining calls to testing.Short where possible. (#110765) * Replace remaining calls to testing.Short where possible. * Update style guide. * Revert change in TestAlertmanager_ExtraDedupStage, as it doesn't work. * Make TestAlertRulePostExport into integration test. --- contribute/backend/style-guide.md | 7 +- .../commands/install_command_test.go | 83 +++++++++---------- .../apis/iam/resourcepermission/sql_test.go | 6 +- .../storage_backend_test.go | 4 - .../resourcepermissions/store_test.go | 4 - .../loki/historian_store_test.go | 3 +- .../ngalert/store/provisioning_store_test.go | 6 +- pkg/tests/alertmanager/alertmanager_test.go | 1 + pkg/tests/api/alerting/api_ruler_test.go | 2 +- pkg/tests/api/folders/api_folders_test.go | 10 ++- .../iam/service_account_integration_test.go | 6 +- pkg/tests/apis/plugins/discovery_test.go | 8 +- pkg/tests/apis/provisioning/exportjob_test.go | 6 +- pkg/tests/testinfra/testinfra.go | 11 ++- pkg/tsdb/influxdb/fsql/fsql_test.go | 2 +- pkg/tsdb/mysql/mysql_snapshot_test.go | 5 +- 16 files changed, 76 insertions(+), 88 deletions(-) diff --git a/contribute/backend/style-guide.md b/contribute/backend/style-guide.md index 20a44414d51..dec2ea1db10 100644 --- a/contribute/backend/style-guide.md +++ b/contribute/backend/style-guide.md @@ -60,13 +60,12 @@ You only need to define `TestMain` in one `_test.go` file within each package. We run unit and integration tests separately, to help keep our CI pipeline running smoothly and provide a better developer experience. -To properly mark a test as being an integration test, you must format your test function definition as follows, with the function name starting with `TestIntegration` and the check for `testing.Short()`: +To properly mark a test as being an integration test, you must format your test function definition as follows, with the function name starting with `TestIntegration` and the check for running in Short mode by using `testutil.SkipIntegrationTestInShortMode(t)` function: ```go func TestIntegrationFoo(t *testing.T) { - if testing.Short() { - t.Skip("skipping integration test") - } + testutil.SkipIntegrationTestInShortMode(t) + // function body } ``` diff --git a/pkg/cmd/grafana-cli/commands/install_command_test.go b/pkg/cmd/grafana-cli/commands/install_command_test.go index 9fcf045d70b..6c4abef5d86 100644 --- a/pkg/cmd/grafana-cli/commands/install_command_test.go +++ b/pkg/cmd/grafana-cli/commands/install_command_test.go @@ -4,11 +4,13 @@ import ( "fmt" "testing" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" + "github.com/grafana/grafana/pkg/cmd/grafana-cli/commands/commandstest" "github.com/grafana/grafana/pkg/cmd/grafana-cli/utils" "github.com/grafana/grafana/pkg/tests/testinfra" - "github.com/stretchr/testify/mock" - "github.com/stretchr/testify/require" + "github.com/grafana/grafana/pkg/util/testutil" ) func TestValidateInput(t *testing.T) { @@ -81,6 +83,43 @@ func TestValidateInput(t *testing.T) { } } +func TestIntegrationPluginRepoConfig(t *testing.T) { + testutil.SkipIntegrationTestInShortMode(t) + + t.Run("Should use config parameter if it is set", func(t *testing.T) { + grafDir, cfgPath := testinfra.CreateGrafDir(t, testinfra.GrafanaOpts{ + GrafanaComAPIURL: "https://grafana-dev.com", + GrafanaComSSOAPIToken: "token3", + }) + + c, err := commandstest.NewCliContext(map[string]string{ + "config": cfgPath, + "homepath": grafDir, + }) + require.NoError(t, err) + repoURL := c.PluginRepoURL() + require.Equal(t, "https://grafana-dev.com/plugins", repoURL) + + token := c.GcomToken() + require.Equal(t, "token3", token) + }) + + t.Run("Should use config overrides parameter if it is set alongside config parameter", func(t *testing.T) { + // GrafanaComApiUrl is set to the default path https://grafana.com/api + grafDir, cfgPath := testinfra.CreateGrafDir(t, testinfra.GrafanaOpts{}) + + // overriding the GrafanaComApiUrl to https://grafana-dev.com + c, err := commandstest.NewCliContext(map[string]string{ + "config": cfgPath, + "homepath": grafDir, + "configOverrides": "cfg:grafana_com.api_url=https://grafana-dev.com", + }) + require.NoError(t, err) + repoURL := c.PluginRepoURL() + require.Equal(t, "https://grafana-dev.com/plugins", repoURL) + }) +} + func TestValidatePluginRepoConfig(t *testing.T) { t.Run("Should use provided repo parameter for installation", func(t *testing.T) { c, err := commandstest.NewCliContext(map[string]string{ @@ -100,44 +139,4 @@ func TestValidatePluginRepoConfig(t *testing.T) { require.NoError(t, err) require.Equal(t, "https://example.com", c.PluginRepoURL()) }) - - t.Run("Should use config parameter if it is set", func(t *testing.T) { - if testing.Short() { - t.Skip("skipping integration test") - } - grafDir, cfgPath := testinfra.CreateGrafDir(t, testinfra.GrafanaOpts{ - GrafanaComAPIURL: "https://grafana-dev.com", - GrafanaComSSOAPIToken: "token3", - }) - - c, err := commandstest.NewCliContext(map[string]string{ - "config": cfgPath, - "homepath": grafDir, - }) - require.NoError(t, err) - repoURL := c.PluginRepoURL() - require.Equal(t, "https://grafana-dev.com/plugins", repoURL) - - token := c.GcomToken() - require.Equal(t, "token3", token) - }) - - t.Run("Should use config overrides parameter if it is set alongside config parameter", func(t *testing.T) { - if testing.Short() { - t.Skip("skipping integration test") - } - - // GrafanaComApiUrl is set to the default path https://grafana.com/api - grafDir, cfgPath := testinfra.CreateGrafDir(t, testinfra.GrafanaOpts{}) - - // overriding the GrafanaComApiUrl to https://grafana-dev.com - c, err := commandstest.NewCliContext(map[string]string{ - "config": cfgPath, - "homepath": grafDir, - "configOverrides": "cfg:grafana_com.api_url=https://grafana-dev.com", - }) - require.NoError(t, err) - repoURL := c.PluginRepoURL() - require.Equal(t, "https://grafana-dev.com/plugins", repoURL) - }) } diff --git a/pkg/registry/apis/iam/resourcepermission/sql_test.go b/pkg/registry/apis/iam/resourcepermission/sql_test.go index f96d2f6d924..13a1c3e6df8 100644 --- a/pkg/registry/apis/iam/resourcepermission/sql_test.go +++ b/pkg/registry/apis/iam/resourcepermission/sql_test.go @@ -206,10 +206,8 @@ func TestIntegration_ResourcePermSqlBackend_getResourcePermission(t *testing.T) } } -func TestResourcePermSqlBackend_deleteResourcePermission(t *testing.T) { - if testing.Short() { - t.Skip("skipping integration test in short mode") - } +func TestIntegrationResourcePermSqlBackend_deleteResourcePermission(t *testing.T) { + testutil.SkipIntegrationTestInShortMode(t) backend := setupBackend(t) sql, err := backend.dbProvider(context.Background()) diff --git a/pkg/registry/apis/iam/resourcepermission/storage_backend_test.go b/pkg/registry/apis/iam/resourcepermission/storage_backend_test.go index 58a09f97a3f..ae4337a1849 100644 --- a/pkg/registry/apis/iam/resourcepermission/storage_backend_test.go +++ b/pkg/registry/apis/iam/resourcepermission/storage_backend_test.go @@ -330,10 +330,6 @@ func TestWriteEvent_Add(t *testing.T) { } func TestWriteEvent_Delete(t *testing.T) { - if testing.Short() { - t.Skip("skipping integration test in short mode") - } - backend := setupBackend(t) sql, err := backend.dbProvider(context.Background()) require.NoError(t, err) diff --git a/pkg/services/accesscontrol/resourcepermissions/store_test.go b/pkg/services/accesscontrol/resourcepermissions/store_test.go index e7ff8fddb8b..359002d0795 100644 --- a/pkg/services/accesscontrol/resourcepermissions/store_test.go +++ b/pkg/services/accesscontrol/resourcepermissions/store_test.go @@ -753,10 +753,6 @@ func retrievePermissionsHelper(store *store, t *testing.T) []orgPermission { } func TestStore_StoreActionSet(t *testing.T) { - if testing.Short() { - t.Skip("skipping integration test") - } - type actionSetTest struct { desc string resource string diff --git a/pkg/services/annotations/annotationsimpl/loki/historian_store_test.go b/pkg/services/annotations/annotationsimpl/loki/historian_store_test.go index 0d509b04b91..967aa88c1d9 100644 --- a/pkg/services/annotations/annotationsimpl/loki/historian_store_test.go +++ b/pkg/services/annotations/annotationsimpl/loki/historian_store_test.go @@ -10,11 +10,12 @@ import ( "testing" "time" - "github.com/grafana/grafana/pkg/services/ngalert/lokiclient" "github.com/prometheus/client_golang/prometheus" "github.com/stretchr/testify/require" "golang.org/x/exp/maps" + "github.com/grafana/grafana/pkg/services/ngalert/lokiclient" + "github.com/grafana/grafana/pkg/components/simplejson" "github.com/grafana/grafana/pkg/infra/db" "github.com/grafana/grafana/pkg/infra/log" diff --git a/pkg/services/ngalert/store/provisioning_store_test.go b/pkg/services/ngalert/store/provisioning_store_test.go index 7d8431474eb..b6f8b8fe5cd 100644 --- a/pkg/services/ngalert/store/provisioning_store_test.go +++ b/pkg/services/ngalert/store/provisioning_store_test.go @@ -156,10 +156,8 @@ func TestIntegrationProvisioningStore(t *testing.T) { } } -func TestSetProvenance_DeadlockScenarios(t *testing.T) { - if testing.Short() { - t.Skip("skipping integration test") - } +func TestIntegrationSetProvenance_DeadlockScenarios(t *testing.T) { + testutil.SkipIntegrationTestInShortMode(t) ng, dbStore := tests.SetupTestEnv(t, testAlertingIntervalSeconds) dbStore.FeatureToggles = featuremgmt.WithFeatures(featuremgmt.FlagAlertingProvenanceLockWrites) diff --git a/pkg/tests/alertmanager/alertmanager_test.go b/pkg/tests/alertmanager/alertmanager_test.go index e3a001d7657..eeac4b90f11 100644 --- a/pkg/tests/alertmanager/alertmanager_test.go +++ b/pkg/tests/alertmanager/alertmanager_test.go @@ -8,6 +8,7 @@ import ( ) func TestAlertmanager_ExtraDedupStage(t *testing.T) { + // TODO: rename test and call testutil.SkipIntegrationTestInShortMode(t) if testing.Short() { t.Skip("skipping integration test") } diff --git a/pkg/tests/api/alerting/api_ruler_test.go b/pkg/tests/api/alerting/api_ruler_test.go index 7804919140c..18eb018b336 100644 --- a/pkg/tests/api/alerting/api_ruler_test.go +++ b/pkg/tests/api/alerting/api_ruler_test.go @@ -711,7 +711,7 @@ func TestIntegrationAlertRuleNestedPermissions(t *testing.T) { }) } -func TestAlertRulePostExport(t *testing.T) { +func TestIntegrationAlertRulePostExport(t *testing.T) { testinfra.SQLiteIntegrationTest(t) // Setup Grafana and its Database diff --git a/pkg/tests/api/folders/api_folders_test.go b/pkg/tests/api/folders/api_folders_test.go index ddcf722cc49..eb6bdd44f2a 100644 --- a/pkg/tests/api/folders/api_folders_test.go +++ b/pkg/tests/api/folders/api_folders_test.go @@ -8,6 +8,7 @@ import ( "github.com/grafana/grafana-openapi-client-go/client/folders" "github.com/grafana/grafana-openapi-client-go/models" + "github.com/grafana/grafana/pkg/services/accesscontrol/resourcepermissions" "github.com/grafana/grafana/pkg/services/featuremgmt" "github.com/grafana/grafana/pkg/services/folder" @@ -17,6 +18,8 @@ import ( "github.com/grafana/grafana/pkg/tests/testinfra" "github.com/grafana/grafana/pkg/tests/testsuite" "github.com/grafana/grafana/pkg/util/retryer" + "github.com/grafana/grafana/pkg/util/testutil" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -25,10 +28,9 @@ func TestMain(m *testing.M) { testsuite.Run(m) } -func TestGetFolders(t *testing.T) { - if testing.Short() { - t.Skip("skipping integration test") - } +func TestIntegrationGetFolders(t *testing.T) { + testutil.SkipIntegrationTestInShortMode(t) + // Setup Grafana and its Database dir, p := testinfra.CreateGrafDir(t, testinfra.GrafanaOpts{ DisableLegacyAlerting: true, diff --git a/pkg/tests/apis/iam/service_account_integration_test.go b/pkg/tests/apis/iam/service_account_integration_test.go index 038c163e3b6..6117a56c2fa 100644 --- a/pkg/tests/apis/iam/service_account_integration_test.go +++ b/pkg/tests/apis/iam/service_account_integration_test.go @@ -15,6 +15,8 @@ import ( "github.com/grafana/grafana/pkg/setting" "github.com/grafana/grafana/pkg/tests/apis" "github.com/grafana/grafana/pkg/tests/testinfra" + "github.com/grafana/grafana/pkg/util/testutil" + "github.com/stretchr/testify/require" "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/runtime/schema" @@ -27,9 +29,7 @@ var gvrServiceAccounts = schema.GroupVersionResource{ } func TestIntegrationServiceAccounts(t *testing.T) { - if testing.Short() { - t.Skip("skipping integration test") - } + testutil.SkipIntegrationTestInShortMode(t) // TODO: Figure out why rest.Mode4 is failing modes := []rest.DualWriterMode{rest.Mode0, rest.Mode1, rest.Mode2, rest.Mode3} diff --git a/pkg/tests/apis/plugins/discovery_test.go b/pkg/tests/apis/plugins/discovery_test.go index 6df8501f4c0..545f2f3bf7d 100644 --- a/pkg/tests/apis/plugins/discovery_test.go +++ b/pkg/tests/apis/plugins/discovery_test.go @@ -4,12 +4,12 @@ import ( "testing" "github.com/stretchr/testify/require" + + "github.com/grafana/grafana/pkg/util/testutil" ) -func TestPluginsIntegrationDiscovery(t *testing.T) { - if testing.Short() { - t.Skip("skipping integration test") - } +func TestIntegrationPluginsIntegrationDiscovery(t *testing.T) { + testutil.SkipIntegrationTestInShortMode(t) t.Run("discovery", func(t *testing.T) { helper := setupHelper(t) diff --git a/pkg/tests/apis/provisioning/exportjob_test.go b/pkg/tests/apis/provisioning/exportjob_test.go index 4eb7826a8f7..665accd84e2 100644 --- a/pkg/tests/apis/provisioning/exportjob_test.go +++ b/pkg/tests/apis/provisioning/exportjob_test.go @@ -16,10 +16,8 @@ import ( "github.com/grafana/grafana/pkg/util/testutil" ) -func TestProvisioning_ExportUnifiedToRepository(t *testing.T) { - if testing.Short() { - t.Skip("skipping integration test") - } +func TestIntegrationProvisioning_ExportUnifiedToRepository(t *testing.T) { + testutil.SkipIntegrationTestInShortMode(t) helper := runGrafana(t) ctx := context.Background() diff --git a/pkg/tests/testinfra/testinfra.go b/pkg/tests/testinfra/testinfra.go index bb1073c9d8f..0b026b7e185 100644 --- a/pkg/tests/testinfra/testinfra.go +++ b/pkg/tests/testinfra/testinfra.go @@ -12,14 +12,16 @@ import ( "testing" "time" - "github.com/grafana/grafana/pkg/configprovider" - "github.com/grafana/grafana/pkg/services/featuremgmt" - "github.com/grafana/grafana/pkg/services/sqlstore/sqlutil" "github.com/prometheus/client_golang/prometheus" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "gopkg.in/ini.v1" + "github.com/grafana/grafana/pkg/configprovider" + "github.com/grafana/grafana/pkg/services/featuremgmt" + "github.com/grafana/grafana/pkg/services/sqlstore/sqlutil" + "github.com/grafana/grafana/pkg/util/testutil" + "github.com/grafana/dskit/kv" "github.com/grafana/grafana/pkg/api" @@ -570,8 +572,9 @@ func CreateGrafDir(t *testing.T, opts GrafanaOpts) (string, string) { func SQLiteIntegrationTest(t *testing.T) { t.Helper() + testutil.SkipIntegrationTestInShortMode(t) - if testing.Short() || !db.IsTestDbSQLite() { + if !db.IsTestDbSQLite() { t.Skip("skipping integration test") } } diff --git a/pkg/tsdb/influxdb/fsql/fsql_test.go b/pkg/tsdb/influxdb/fsql/fsql_test.go index ed7b722a6b2..562710aa24c 100644 --- a/pkg/tsdb/influxdb/fsql/fsql_test.go +++ b/pkg/tsdb/influxdb/fsql/fsql_test.go @@ -55,7 +55,7 @@ func (suite *FSQLTestSuite) AfterTest(suiteName, testName string) { suite.server.Shutdown() } -func TestFSQLTestSuite(t *testing.T) { +func TestIntegrationFSQLTestSuite(t *testing.T) { if testing.Short() { t.Skip("skipping integration test in short mode") } diff --git a/pkg/tsdb/mysql/mysql_snapshot_test.go b/pkg/tsdb/mysql/mysql_snapshot_test.go index 16de2efe46d..3b8c30ee7f7 100644 --- a/pkg/tsdb/mysql/mysql_snapshot_test.go +++ b/pkg/tsdb/mysql/mysql_snapshot_test.go @@ -36,11 +36,8 @@ func TestIntegrationMySQLSnapshots(t *testing.T) { if testing.Short() { t.Skip("skipping integration test in short mode") } - shouldRunTest := func() bool { - if testing.Short() { - return false - } + shouldRunTest := func() bool { testDbName, present := os.LookupEnv("GRAFANA_TEST_DB") if present && testDbName == "mysql" {