From ee8bd4ece43792bf7310344dcebf4fdac7c38c37 Mon Sep 17 00:00:00 2001 From: Jesse Hallam Date: Tue, 19 May 2020 21:03:02 -0300 Subject: [PATCH] MM-23250: improve config.Migrate tests (#14479) * MM-23250: improve config.Migrate tests The config.Migrate tests were not running in isolation, but wrote over the "local" config. The test coverage for both directions was not exhaustive, and lacked coverage for the recently added plugin signature files. Rewrite the unit tests as such, fixing a minor issue closing the stores after migration. Fixes: https://mattermost.atlassian.net/browse/MM-23250 * idiomatic err checking * clarify random file contents Co-authored-by: mattermod --- config/migrate.go | 14 ++-- config/migrate_test.go | 166 +++++++++++++++++++++++++++-------------- 2 files changed, 120 insertions(+), 60 deletions(-) diff --git a/config/migrate.go b/config/migrate.go index a300cebb7f..85ba32f374 100644 --- a/config/migrate.go +++ b/config/migrate.go @@ -11,29 +11,33 @@ func Migrate(from, to string) error { if err != nil { return errors.Wrapf(err, "failed to access source config %s", from) } + defer source.Close() destination, err := NewStore(to, false) if err != nil { return errors.Wrapf(err, "failed to access destination config %s", to) } + defer destination.Close() sourceConfig := source.Get() if _, err = destination.Set(sourceConfig); err != nil { return errors.Wrapf(err, "failed to set config") } - files := []string{*sourceConfig.SamlSettings.IdpCertificateFile, *sourceConfig.SamlSettings.PublicCertificateFile, - *sourceConfig.SamlSettings.PrivateKeyFile} + files := []string{ + *sourceConfig.SamlSettings.IdpCertificateFile, + *sourceConfig.SamlSettings.PublicCertificateFile, + *sourceConfig.SamlSettings.PrivateKeyFile, + } files = append(files, sourceConfig.PluginSettings.SignaturePublicKeyFiles...) for _, file := range files { - err = migrateFile(file, source, destination) - - if err != nil { + if err := migrateFile(file, source, destination); err != nil { return err } } + return nil } diff --git a/config/migrate_test.go b/config/migrate_test.go index 618bc78fb6..974cffa14a 100644 --- a/config/migrate_test.go +++ b/config/migrate_test.go @@ -1,80 +1,136 @@ // Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. // See LICENSE.txt for license information. -package config +package config_test import ( + "io/ioutil" + "os" + "path" "testing" + "github.com/mattermost/mattermost-server/v5/config" "github.com/mattermost/mattermost-server/v5/model" - "github.com/mattermost/mattermost-server/v5/testlib" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) -func getDsn(driver string, source string) string { - if driver == model.DATABASE_DRIVER_MYSQL { - return driver + "://" + source +func TestMigrate(t *testing.T) { + files := []string{ + "IdpCertificateFile", + "PublicCertificateFile", + "PrivateKeyFile", + "internal.crt", + "internal2.crt", } - return source -} - -func TestMigrateDatabaseToFile(t *testing.T) { - if testing.Short() { - t.SkipNow() + filesData := make([]string, len(files)) + for i := range files { + // Generate random data for each file, ensuring that stale data from a past test + // won't generate a false positive. + filesData[i] = model.NewId() } - helper := testlib.NewMainHelper() - sqlSettings := helper.GetSQLSettings() - fileDSN := "config.json" - files := []string{"IdpCertificateFile", "PublicCertificateFile", "PrivateKeyFile"} - data := []byte("aaaaa") - ds, err := NewDatabaseStore(getDsn(*sqlSettings.DriverName, *sqlSettings.DataSource)) - defer ds.Close() - defer func() { - defaultCfg := &model.Config{} - defaultCfg.SetDefaults() - ds.Set(defaultCfg) - }() - require.NoError(t, err) - config := ds.Get() - config.SamlSettings.IdpCertificateFile = &files[0] - config.SamlSettings.PublicCertificateFile = &files[1] - config.SamlSettings.PrivateKeyFile = &files[2] - _, err = ds.Set(config) - require.NoError(t, err) - for _, file := range files { - err = ds.SetFile(file, data) + setup := func(t *testing.T) { + os.Clearenv() + t.Helper() + + tempDir, err := ioutil.TempDir("", "TestMigrate") require.NoError(t, err) - } - err = Migrate(getDsn(*sqlSettings.DriverName, *sqlSettings.DataSource), fileDSN) - require.NoError(t, err) + t.Cleanup(func() { + os.RemoveAll(tempDir) + }) - fs, err := NewFileStore(fileDSN, false) - require.NoError(t, err) - defer fs.Close() - - for _, file := range files { - hasFile, err := fs.HasFile(file) + err = os.Chdir(tempDir) require.NoError(t, err) - defer fs.RemoveFile(file) - assert.True(t, hasFile) + + truncateTables(t) } - assert.Equal(t, ds.Get(), fs.Get()) -} + setupSource := func(t *testing.T, source config.Store) { + t.Helper() -func TestMigrateFileToDatabaseWhenFilePathIsNotSpecified(t *testing.T) { - if testing.Short() { - t.SkipNow() + cfg := source.Get() + cfg.ServiceSettings.SiteURL = sToP("http://example.com") + cfg.SamlSettings.IdpCertificateFile = &files[0] + cfg.SamlSettings.PublicCertificateFile = &files[1] + cfg.SamlSettings.PrivateKeyFile = &files[2] + cfg.PluginSettings.SignaturePublicKeyFiles = []string{ + files[3], + files[4], + } + + _, err := source.Set(cfg) + require.NoError(t, err) + + for i, file := range files { + err = source.SetFile(file, []byte(filesData[i])) + require.NoError(t, err) + } } - helper := testlib.NewMainHelper() - sqlSettings := helper.GetSQLSettings() - fileDSN := "config.json" - _, err := NewFileStore(fileDSN, true) - require.NoError(t, err) + assertDestination := func(t *testing.T, destination config.Store, source config.Store) { + t.Helper() - err = Migrate(fileDSN, getDsn(*sqlSettings.DriverName, *sqlSettings.DataSource)) - require.NoError(t, err) + for i, file := range files { + hasFile, err := destination.HasFile(file) + require.NoError(t, err) + require.Truef(t, hasFile, "destination missing file %s", file) + + actualData, err := destination.GetFile(file) + require.NoError(t, err) + assert.Equalf(t, []byte(filesData[i]), actualData, "destination has wrong contents for file %s", file) + } + + assert.Equal(t, source.Get(), destination.Get()) + } + + t.Run("database to file", func(t *testing.T) { + setup(t) + + pwd, err := os.Getwd() + require.NoError(t, err) + + sqlSettings := mainHelper.GetSQLSettings() + destinationDSN := path.Join(pwd, "config-custom.json") + sourceDSN := getDsn(*sqlSettings.DriverName, *sqlSettings.DataSource) + + source, err := config.NewDatabaseStore(sourceDSN) + require.NoError(t, err) + defer source.Close() + + setupSource(t, source) + err = config.Migrate(sourceDSN, destinationDSN) + require.NoError(t, err) + + destination, err := config.NewFileStore(destinationDSN, false) + require.NoError(t, err) + defer destination.Close() + + assertDestination(t, destination, source) + }) + + t.Run("file to database", func(t *testing.T) { + setup(t) + + pwd, err := os.Getwd() + require.NoError(t, err) + + sqlSettings := mainHelper.GetSQLSettings() + sourceDSN := path.Join(pwd, "config-custom.json") + destinationDSN := getDsn(*sqlSettings.DriverName, *sqlSettings.DataSource) + + source, err := config.NewFileStore(sourceDSN, false) + require.NoError(t, err) + defer source.Close() + + setupSource(t, source) + err = config.Migrate(sourceDSN, destinationDSN) + require.NoError(t, err) + + destination, err := config.NewDatabaseStore(destinationDSN) + require.NoError(t, err) + defer destination.Close() + + assertDestination(t, destination, source) + }) }