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

Simplify testing for Cloud Posse components #41

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions pkg/atmos/aws-component-helper/atmos.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,16 @@ import (
"github.com/stretchr/testify/require"
)

func GetAtmosTestOptions(t *testing.T, componentName string, stackName string, vars map[string]interface{}) *atmos.Options {
atmosOptions := atmos.WithDefaultRetryableErrors(t, &atmos.Options{
Component: componentName,
Stack: stackName,
NoColor: false,
Vars: vars,
})
return atmosOptions
}

func GetAtmosOptions(t *testing.T, suite *TestSuite, componentName string, stackName string, vars map[string]interface{}) *atmos.Options {
mergedVars := map[string]interface{}{
"attributes": []string{suite.RandomIdentifier},
Expand Down
106 changes: 106 additions & 0 deletions pkg/atmos/aws-component-helper/setup_subject.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,106 @@
package aws_component_helper

import (
"github.com/gruntwork-io/terratest/modules/logger"
"github.com/stretchr/testify/require"
"os"
"path/filepath"
"strings"
"testing"

"github.com/cloudposse/test-helpers/pkg/atmos"
"github.com/gruntwork-io/terratest/modules/files"
"github.com/stretchr/testify/assert"
)

func awsComponentTestCleanup(t *testing.T, opts *atmos.Options, destroy bool, tmpDir string, protectDir string) {
if destroy {
out, err := atmos.DestroyE(t, opts)
if err == nil {
// if the destroy was successful, remove the temp directory
if tmpDir == protectDir {
t.Logf("Not removing protected directory directory %s", protectDir)
Nuru marked this conversation as resolved.
Show resolved Hide resolved
} else {
t.Log("Cleaning out temp folder...")
err = os.RemoveAll(tmpDir)
if err == nil {
t.Logf("Removed temp directory %s", tmpDir)
} else {
assert.NoError(t, err, "Failed to remove temp directory %s", tmpDir)
Nuru marked this conversation as resolved.
Show resolved Hide resolved
}
}
} else {
// if the destroy failed, leave the temp directory in place
assert.NoError(t, err, "Failed to destroy subject. Leaving source and state in %v\n\nDestroy output:\n%v\n\n", tmpDir, out)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Avoid failing the test during cleanup when destroy operation fails

If the destroy operation fails during cleanup, using assert.NoError will fail the test at the end. Consider logging the error to report the issue without failing the entire test.

Apply this diff to log the error instead:

-    			assert.NoError(t, err, "Failed to destroy subject. Leaving source and state in %v\n\nDestroy output:\n%v\n\n", tmpDir, out)
+    			t.Errorf("Failed to destroy subject. Leaving source and state in %v\n\nDestroy output:\n%v\n\nError: %v", tmpDir, out, err)
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
assert.NoError(t, err, "Failed to destroy subject. Leaving source and state in %v\n\nDestroy output:\n%v\n\n", tmpDir, out)
t.Errorf("Failed to destroy subject. Leaving source and state in %v\n\nDestroy output:\n%v\n\nError: %v", tmpDir, out, err)

}
}
}

type ComponentTestResults struct {
Output string
TestID string
}

func IsLocalAwsComponentTest(t *testing.T) bool {
atmosPlainOpts := &atmos.Options{}
existingTestID, err := atmos.RunAtmosCommandE(t, atmosPlainOpts, "test", "get-test-id")

return err == nil && len(existingTestID) > 0
}

func AwsComponentTestHelper(t *testing.T, opts *atmos.Options, callback func(t *testing.T, opts *atmos.Options, results ComponentTestResults)) {
testSrcRoot := os.Getenv("ATMOS_BASE_PATH")
testRoot := testSrcRoot
if testSrcRoot == "" {
assert.FailNow(t, "ATMOS_BASE_PATH must be set, but is empty")
}

atmosPlainOpts := &atmos.Options{}
doApply := false

var testID string

existingTestID, err := atmos.RunAtmosCommandE(t, atmosPlainOpts, "test", "get-test-id")
if err != nil || len(existingTestID) == 0 {
doApply = true
// Copy test source to a temp directory and create a new test ID
t.Log("Copying files to temp folder...")
testRoot, err = files.CopyTerraformFolderToTemp(testSrcRoot, t.Name())
require.NoError(t, err)
err = copyDirectoryRecursively(filepath.Join(testSrcRoot, "state"), filepath.Join(testRoot, "state"))
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Ensure copyDirectoryRecursively function is defined or imported

The function copyDirectoryRecursively used on line 70 is not defined in this file or imported from any package. This will result in a build error.

Please define the copyDirectoryRecursively function or import it from the appropriate package to ensure the code compiles correctly.

require.NoError(t, err)
atmosPlainOpts.AtmosBasePath = testRoot
testID, err = atmos.RunAtmosCommandE(t, atmosPlainOpts, "test", "make-test-id")
require.NoError(t, err)
testID = strings.TrimSpace(testID)
} else {
testID = strings.TrimSpace(existingTestID)
}

t.Logf("Running test \"%s\" with test ID \"%s\" in directory %s", t.Name(), testID, testRoot)

options := atmos.WithDefaultRetryableErrors(t, opts)
options.AtmosBasePath = testRoot
// Keep the output quiet
if !testing.Verbose() {
options.Logger = logger.Discard
}

defer awsComponentTestCleanup(t, options, doApply, testRoot, testSrcRoot)

// Apply the deployment
out := ""
if doApply {
out, err = atmos.ApplyE(t, options)
}
require.NoError(t, err, "Failed to deploy component, skipping other tests.")
Nuru marked this conversation as resolved.
Show resolved Hide resolved
// Call the callback function for assertions
callback(t, options, ComponentTestResults{
Output: out,
TestID: testID,
})

if !doApply {
t.Logf("\n\n\nTests complete in %s\n\n", testRoot)
}
}
17 changes: 17 additions & 0 deletions pkg/atmos/cmd.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,16 @@ import (
)

func generateCommand(options *Options, args ...string) shell.Command {
// TODO: replace with command line argument
if options.AtmosBasePath != "" {
var err error
options, err = options.Clone()
if err != nil {
fmt.Printf("Error cloning options: %v", err)
return shell.Command{}
}
Comment on lines +21 to +24
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Improve error handling for clone failure

Returning an empty shell.Command{} when cloning fails could lead to silent failures downstream. Consider either:

  1. Propagating the error up the call chain, or
  2. Setting a default fallback behavior
- if err != nil {
-   fmt.Printf("Error cloning options: %v", err)
-   return shell.Command{}
- }
+ if err != nil {
+   // Either propagate the error:
+   panic(fmt.Sprintf("Failed to clone options: %v", err))
+   // Or use original options as fallback:
+   options.EnvVars["ATMOS_BASE_PATH"] = options.AtmosBasePath
+ }

Committable suggestion skipped: line range outside the PR's diff.

options.EnvVars["ATMOS_BASE_PATH"] = options.AtmosBasePath
}
cmd := shell.Command{
Command: options.AtmosBinary,
Args: args,
Expand Down Expand Up @@ -43,6 +53,13 @@ func GetCommonOptions(options *Options, args ...string) (*Options, []string) {
options.AtmosBinary = DefaultExecutable
}

// TODO:
// if options.AtmosBasePath != "", then add args "--base-path", options.AtmosBasePath to the args
// Unfortunately, as of Atmos 1.109.0, the flag "--base-path" does not work
// if options.AtmosBasePath != "" {
// args = append([]string{"--base-path", options.AtmosBasePath}, args...)
// }

Comment on lines +56 to +62
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Remove commented code and track the issue properly

The commented code block indicates that the --base-path flag doesn't work in Atmos 1.109.0, but leaves the code in place without proper tracking. Instead of keeping commented code:

  1. Remove the commented code
  2. Create an issue to track when this will be supported
  3. Document the current workaround (using ATMOS_BASE_PATH environment variable)
- // TODO:
- // if options.AtmosBasePath != "", then add args "--base-path", options.AtmosBasePath to the args
- // Unfortunately, as of Atmos 1.109.0, the flag "--base-path" does not work
- // if options.AtmosBasePath != "" {
- // 	args = append([]string{"--base-path", options.AtmosBasePath}, args...)
- // }

+ // NOTE: Atmos base path is currently set via ATMOS_BASE_PATH environment variable in generateCommand()
+ // as the --base-path flag is not supported as of Atmos 1.109.0
+ // Track progress at: <link-to-issue>
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
// TODO:
// if options.AtmosBasePath != "", then add args "--base-path", options.AtmosBasePath to the args
// Unfortunately, as of Atmos 1.109.0, the flag "--base-path" does not work
// if options.AtmosBasePath != "" {
// args = append([]string{"--base-path", options.AtmosBasePath}, args...)
// }
// NOTE: Atmos base path is currently set via ATMOS_BASE_PATH environment variable in generateCommand()
// as the --base-path flag is not supported as of Atmos 1.109.0
// Track progress at: <link-to-issue>

if options.Parallelism > 0 && len(args) > 0 && args[0] == "terraform" && collections.ListContains(commandsWithParallelism, args[1]) {
args = append(args, fmt.Sprintf("--parallelism=%d", options.Parallelism))
}
Expand Down
Loading