refactor test helpers #44
Conversation
|
Warning Rate limit exceeded@mcalhoun has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 22 minutes and 38 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (32)
WalkthroughThis pull request introduces a significant refactoring of the AWS component testing framework. The primary changes involve renaming the Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 18
🔭 Outside diff range comments (1)
go.mod (1)
Line range hint
3-3: Fix invalid Go version.The specified Go version 1.23 doesn't exist yet (current latest is 1.22). This needs to be corrected to avoid compatibility issues.
-go 1.23 +go 1.22
🧹 Nitpick comments (12)
pkg/atmos/component-helper/bools.go (1)
5-13: Add nil check and consider renaming function for clarityThe function should validate the config parameter and have a more descriptive name that reflects its purpose in checking phase execution status.
Consider this implementation:
-func anyPhasesSkipped(config *config.Config) bool { +func hasSkippedPhases(config *config.Config) bool { + if config == nil { + return false + } return config.SkipDeployComponent || config.SkipDeployDependencies || config.SkipDestroyComponent || config.SkipDestroyDependencies || config.SkipSetupTestSuite || config.SkipTeardownTestSuite || config.SkipVendorDependencies }🧰 Tools
🪛 GitHub Actions: Run Tests
[error] Test suite 'TestExampleTestSuite' failed with exit code 1
pkg/atmos/component-helper/20_enabled_flag_tst.go (2)
11-11: Fix typo in log messageThere's a typo in the log message: "verifiy" should be "verify".
- s.logPhaseStatus("verifiy enabled flag", "skipped") + s.logPhaseStatus("verify enabled flag", "skipped")🧰 Tools
🪛 GitHub Actions: Run Tests
[error] Test suite 'TestExampleTestSuite' failed with exit code 1
15-24: Simplify map merging logicThe current map merging implementation could be simplified and made more robust.
Consider this approach:
- mergedVars := map[string]interface{}{ - "attributes": []string{s.Config.RandomIdentifier}, - "enabled": false, - } - - // Merge in any additional vars passed in - if additionalVars != nil { - err := mergo.Merge(&mergedVars, additionalVars) - require.NoError(s.T(), err) - } + mergedVars := map[string]interface{}{ + "attributes": []string{s.Config.RandomIdentifier}, + "enabled": false, + } + + if additionalVars != nil { + for k, v := range *additionalVars { + mergedVars[k] = v + } + }🧰 Tools
🪛 GitHub Actions: Run Tests
[error] Test suite 'TestExampleTestSuite' failed with exit code 1
pkg/atmos/component-helper/01_deploy.go (1)
12-37: Add structured logging with metadataEnhance the logging to include more context and structured fields.
Consider adding these fields to the log messages:
- s.logPhaseStatus(phaseName, "started") + s.logPhaseStatus(phaseName, "started", "dependencies_count", len(s.Dependencies)) - log.Info("no dependencies to deploy") + log.Info("no dependencies to deploy", "phase", phaseName) - log.Info("deploying dependency", "component", dependency.ComponentName, "stack", dependency.StackName) + log.Info("deploying dependency", + "phase", phaseName, + "component", dependency.ComponentName, + "stack", dependency.StackName, + "vars_count", len(dependency.AdditionalVars))🧰 Tools
🪛 GitHub Actions: Run Tests
[error] Test suite 'TestExampleTestSuite' failed with exit code 1
pkg/atmos/component-helper/config/config.go (2)
145-151: Improve error handling for missing configuration file.The code silently ignores when the config file is not found. This could mask configuration issues and make debugging harder.
Consider logging a warning when the config file is not found:
if err := viper.ReadInConfig(); err != nil { if _, ok := err.(viper.ConfigFileNotFoundError); ok { - // Config file not found; ignore error and use defaults + log.Warn("Config file not found, using default values", + "expected_path", viper.GetString("ConfigFilePath")) } else { t.Fatal(fmt.Errorf("fatal error config file: %w", err)) } }🧰 Tools
🪛 GitHub Actions: Run Tests
[error] Test suite 'TestExampleTestSuite' failed with exit code 1
69-79: Improve flag binding detection logic.The current implementation uses string prefix matching which is fragile and could lead to false positives/negatives.
Consider using a more robust approach:
-// isPFlagBound checks if a key is bound to a pflag -func isPFlagBound(key string) bool { - if key == "configfilepath" { - return false - } - - // Check if the key corresponds to a defined flag - return strings.HasPrefix(key, "skip") || - strings.HasPrefix(key, "test") || - flag.Lookup(key) != nil +// isPFlagBound checks if a key is bound to a pflag +func isPFlagBound(key string) bool { + // Maintain a set of keys that should never be considered as bound + excludedKeys := map[string]bool{ + "configfilepath": true, + } + + if excludedKeys[key] { + return false + } + + // Check if the flag is explicitly defined + return pflag.CommandLine.Lookup(key) != nil }🧰 Tools
🪛 GitHub Actions: Run Tests
[error] Test suite 'TestExampleTestSuite' failed with exit code 1
pkg/atmos/component-helper/00_setup.go (2)
15-45: Enhance directory copying with better error handling and logging.The current implementation could benefit from more robust error handling and progress tracking.
Consider these improvements:
func (s *TestSuite) copyDirectoryContents(srcDir string, destDir string) error { + var fileCount int + var totalSize int64 // Walk through all files and directories in srcDir and copy them to destDir return filepath.Walk(srcDir, func(path string, info os.FileInfo, err error) error { if err != nil { - return err + return fmt.Errorf("failed to access path %s: %w", path, err) } // Calculate the relative path from srcDir relPath, err := filepath.Rel(srcDir, path) if err != nil { - return err + return fmt.Errorf("failed to calculate relative path for %s: %w", path, err) } // Calculate destination path in destDir destPath := filepath.Join(destDir, relPath) if info.IsDir() { + log.Debug("creating directory", "path", destPath) // Create directory in destination - return os.MkdirAll(destPath, info.Mode()) + if err := os.MkdirAll(destPath, info.Mode()); err != nil { + return fmt.Errorf("failed to create directory %s: %w", destPath, err) + } + return nil } else { + fileCount++ + totalSize += info.Size() + if fileCount%100 == 0 { + log.Debug("copying files", "count", fileCount, "total_size_mb", totalSize/1024/1024) + } + // Copy file content content, err := os.ReadFile(path) if err != nil { - return err + return fmt.Errorf("failed to read file %s: %w", path, err) } - return os.WriteFile(destPath, content, info.Mode()) + if err := os.WriteFile(destPath, content, info.Mode()); err != nil { + return fmt.Errorf("failed to write file %s: %w", destPath, err) + } + return nil } }) }🧰 Tools
🪛 GitHub Actions: Run Tests
[error] Test suite 'TestExampleTestSuite' failed with exit code 1
68-97: Reduce duplication in directory setup functions.The
setTempDirandsetStateDirfunctions share similar patterns and could be refactored to reduce duplication.Consider this refactor:
+func setDirectory(t *testing.T, config *c.Config, dirType string, parentDir string) string { + var dirPath string + switch dirType { + case "temp": + if config.TempDir == "" { + var err error + dirPath, err = os.MkdirTemp("", "atmos-test-helper") + require.NoError(t, err) + } else { + dirPath = config.TempDir + } + case "state": + if config.StateDir == "" { + dirPath = filepath.Join(parentDir, "state") + err := os.MkdirAll(dirPath, 0755) + require.NoError(t, err) + } else { + dirPath = config.StateDir + } + } + + viper.Set(dirType+"Dir", dirPath) + if dirType == "temp" { + config.TempDir = dirPath + } else { + config.StateDir = dirPath + } + + err := config.WriteConfig() + require.NoError(t, err) + + log.WithPrefix(t.Name()).Info(fmt.Sprintf("%s directory for tests", dirType), + "path", dirPath) + + return dirPath +} + -func setTempDir(t *testing.T, config *c.Config) { - if config.TempDir == "" { - tempDir, err := os.MkdirTemp("", "atmos-test-helper") - require.NoError(t, err) - - viper.Set("TempDir", tempDir) - config.TempDir = tempDir - - err = config.WriteConfig() - require.NoError(t, err) - } - - log.WithPrefix(t.Name()).Info("tests will be run in temp directory", "path", config.TempDir) +func setTempDir(t *testing.T, config *c.Config) { + setDirectory(t, config, "temp", "") } -func setStateDir(t *testing.T, config *c.Config) { - if config.StateDir == "" { - stateDir := filepath.Join(config.TempDir, "state") - viper.Set("StateDir", stateDir) - config.StateDir = stateDir - - err := os.MkdirAll(stateDir, 0755) - require.NoError(t, err) - - err = config.WriteConfig() - require.NoError(t, err) - } - - log.WithPrefix(t.Name()).Info("terraform state for tests will be saved in state directory", "path", config.StateDir) +func setStateDir(t *testing.T, config *c.Config) { + setDirectory(t, config, "state", config.TempDir) }🧰 Tools
🪛 GitHub Actions: Run Tests
[error] Test suite 'TestExampleTestSuite' failed with exit code 1
pkg/atmos/component-helper/logger.go (1)
8-13: Consider using constants for status strings.Define constants for status strings to prevent typos and enable compile-time checking.
+const ( + StatusSkipped = "skipped" + StatusCompleted = "completed" + StatusStarted = "started" + StatusFailed = "failed" +) var statusStyles = map[string]lipgloss.Style{ - "skipped": lipgloss.NewStyle().Foreground(lipgloss.Color("11")).Italic(true), - "completed": lipgloss.NewStyle().Foreground(lipgloss.Color("10")).Bold(true), - "started": lipgloss.NewStyle().Foreground(lipgloss.Color("14")), - "failed": lipgloss.NewStyle().Foreground(lipgloss.Color("9")).Bold(true).Underline(true), + StatusSkipped: lipgloss.NewStyle().Foreground(lipgloss.Color("11")).Italic(true), + StatusCompleted: lipgloss.NewStyle().Foreground(lipgloss.Color("10")).Bold(true), + StatusStarted: lipgloss.NewStyle().Foreground(lipgloss.Color("14")), + StatusFailed: lipgloss.NewStyle().Foreground(lipgloss.Color("9")).Bold(true).Underline(true), }🧰 Tools
🪛 GitHub Actions: Run Tests
[error] Test suite 'TestExampleTestSuite' failed with exit code 1
pkg/atmos/component-helper/10_vendor.go (1)
14-25: Add atomic file check to prevent race conditions.The current implementation could lead to race conditions in concurrent tests.
func vendorFileExists(t *testing.T, config *c.Config) bool { vendorFile := filepath.Join(config.TempDir, "vendor.yaml") - _, err := os.Stat(vendorFile) + f, err := os.Open(vendorFile) if os.IsNotExist(err) { log.WithPrefix(t.Name()).Warn("skipping test phase because vendor file does not exist", "phase", "setup test suite/vendor dependencies", "vendorFile", vendorFile) return false } + if err == nil { + f.Close() + } require.NoError(t, err) return true }🧰 Tools
🪛 GitHub Actions: Run Tests
[error] Test suite 'TestExampleTestSuite' failed with exit code 1
pkg/atmos/component-helper/99_destroy.go (1)
13-30: Add file existence check before removal.The function attempts to remove the config file without first checking if it exists. This could lead to unnecessary error handling for non-existent files.
Consider this improvement:
func (s *TestSuite) DestroyConfigFile(t *testing.T, config *c.Config) { const phaseName = "teardown/destroy config file" if anyPhasesSkipped(config) { s.logPhaseStatus(phaseName, "skipped") return } s.logPhaseStatus(phaseName, "started") + if _, err := os.Stat(config.ConfigFilePath); os.IsNotExist(err) { + log.Info("config file already removed", "path", config.ConfigFilePath) + s.logPhaseStatus(phaseName, "completed") + return + } + err := os.Remove(config.ConfigFilePath) if err != nil { s.logPhaseStatus(phaseName, "failed") require.NoError(t, err) } s.logPhaseStatus(phaseName, "completed") }🧰 Tools
🪛 GitHub Actions: Run Tests
[error] Test suite 'TestExampleTestSuite' failed with exit code 1
pkg/atmos/component-helper/suite.go (1)
16-27: Add documentation for exported types and methods.The
TestSuitestruct and its constructor lack documentation comments, which are essential for exported types in Go.Add documentation:
+// TestSuite provides a test suite for Atmos components with configuration and dependency management. type TestSuite struct { Config *c.Config Dependencies []*dependency.Dependency suite.Suite } +// NewTestSuite creates a new TestSuite instance with initialized dependencies. func NewTestSuite() *TestSuite { tsuite := new(TestSuite) tsuite.Dependencies = make([]*dependency.Dependency, 0) return tsuite }🧰 Tools
🪛 GitHub Actions: Run Tests
[error] Test suite 'TestExampleTestSuite' failed with exit code 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (33)
.gitignore(1 hunks)README.md(1 hunks)README.yaml(2 hunks)go.mod(5 hunks)pkg/atmos/aws-component-helper/atmos.go(0 hunks)pkg/atmos/aws-component-helper/cli.go(0 hunks)pkg/atmos/aws-component-helper/cli_test.go(0 hunks)pkg/atmos/aws-component-helper/const.go(0 hunks)pkg/atmos/aws-component-helper/dependency.go(0 hunks)pkg/atmos/aws-component-helper/setup_component.go(0 hunks)pkg/atmos/aws-component-helper/setup_component_test.go(0 hunks)pkg/atmos/aws-component-helper/setup_test_suite.go(0 hunks)pkg/atmos/aws-component-helper/setup_test_suite_test.go(0 hunks)pkg/atmos/aws-component-helper/shared.go(0 hunks)pkg/atmos/aws-component-helper/tear_down_suite.go(0 hunks)pkg/atmos/aws-component-helper/test_suite.go(0 hunks)pkg/atmos/component-helper/00_setup.go(1 hunks)pkg/atmos/component-helper/01_deploy.go(1 hunks)pkg/atmos/component-helper/10_vendor.go(1 hunks)pkg/atmos/component-helper/20_enabled_flag_tst.go(1 hunks)pkg/atmos/component-helper/99_destroy.go(1 hunks)pkg/atmos/component-helper/README.md(1 hunks)pkg/atmos/component-helper/atmos_options.go(1 hunks)pkg/atmos/component-helper/bools.go(1 hunks)pkg/atmos/component-helper/config/config.go(1 hunks)pkg/atmos/component-helper/dependency/dependency.go(1 hunks)pkg/atmos/component-helper/logger.go(1 hunks)pkg/atmos/component-helper/suite.go(1 hunks)pkg/atmos/component-helper/suite_test.go(1 hunks)test/aws-component-helper/test/basic_test.go(0 hunks)test/component-helper-integration/test/component_helper_integration_test.go(1 hunks)test/component-helper-integration/test/fixtures/stacks/orgs/test/main.yaml(2 hunks)test/component-helper-integration/test/fixtures/vendor.yaml(1 hunks)
💤 Files with no reviewable changes (13)
- pkg/atmos/aws-component-helper/tear_down_suite.go
- pkg/atmos/aws-component-helper/const.go
- pkg/atmos/aws-component-helper/dependency.go
- pkg/atmos/aws-component-helper/setup_component.go
- test/aws-component-helper/test/basic_test.go
- pkg/atmos/aws-component-helper/cli_test.go
- pkg/atmos/aws-component-helper/setup_component_test.go
- pkg/atmos/aws-component-helper/setup_test_suite_test.go
- pkg/atmos/aws-component-helper/shared.go
- pkg/atmos/aws-component-helper/cli.go
- pkg/atmos/aws-component-helper/setup_test_suite.go
- pkg/atmos/aws-component-helper/atmos.go
- pkg/atmos/aws-component-helper/test_suite.go
✅ Files skipped from review due to trivial changes (2)
- .gitignore
- README.md
🧰 Additional context used
🪛 GitHub Actions: Run Tests
pkg/atmos/component-helper/dependency/dependency.go
[error] Test suite 'TestExampleTestSuite' failed with exit code 1
pkg/atmos/component-helper/atmos_options.go
[error] Test suite 'TestExampleTestSuite' failed with exit code 1
pkg/atmos/component-helper/01_deploy.go
[error] Test suite 'TestExampleTestSuite' failed with exit code 1
pkg/atmos/component-helper/suite_test.go
[error] Test suite 'TestExampleTestSuite' failed with exit code 1
pkg/atmos/component-helper/20_enabled_flag_tst.go
[error] Test suite 'TestExampleTestSuite' failed with exit code 1
pkg/atmos/component-helper/logger.go
[error] Test suite 'TestExampleTestSuite' failed with exit code 1
pkg/atmos/component-helper/10_vendor.go
[error] Test suite 'TestExampleTestSuite' failed with exit code 1
pkg/atmos/component-helper/README.md
[error] Test suite 'TestExampleTestSuite' failed with exit code 1
pkg/atmos/component-helper/bools.go
[error] Test suite 'TestExampleTestSuite' failed with exit code 1
pkg/atmos/component-helper/suite.go
[error] Test suite 'TestExampleTestSuite' failed with exit code 1
pkg/atmos/component-helper/99_destroy.go
[error] Test suite 'TestExampleTestSuite' failed with exit code 1
pkg/atmos/component-helper/config/config.go
[error] Test suite 'TestExampleTestSuite' failed with exit code 1
pkg/atmos/component-helper/00_setup.go
[error] 131-131: Test failure: Unable to access '../src' directory - directory does not exist (lstat ../src: no such file or directory)
[error] Test suite 'TestExampleTestSuite' failed with exit code 1
🔇 Additional comments (7)
test/component-helper-integration/test/fixtures/stacks/orgs/test/main.yaml (2)
6-6: LGTM! Region explicitly defined.Good practice to explicitly define the region rather than relying on defaults.
39-53: Verify component dependencies.The components have been restructured with a more consistent naming pattern (component1, component2, component3) and sequential revision numbers. Please ensure that:
- The removal of dep2 doesn't break any existing tests
- The revision number changes (21 → 1) don't cause any compatibility issues
✅ Verification successful
Dependencies verified successfully ✅
The changes are safe as:
- No references to the removed
dep2found in the codebase- Revision numbers are only referenced within the component definitions
- Integration tests properly handle the dependencies between
component1,component2, andcomponent3🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any dependencies on the removed component # Search for dep2 references echo "Searching for dep2 references..." rg -g '*.{yaml,yml,tf,go}' --line-number '\bdep2\b' -A 5 # Search for revision number references echo "Searching for revision number references..." rg -g '*.{yaml,yml,tf,go}' --line-number 'revision:\s*(21|1|2|3)' -A 5Length of output: 1361
Script:
#!/bin/bash # Search for test files containing references to component1, component2, or component3 echo "Searching for component references in test files..." rg -g '*test*' -g '*.{yaml,yml,go}' --line-number 'component[123]' -A 5 # Search for any files containing 'target' component references echo "Searching for target component references..." rg -g '*.{yaml,yml,go}' --line-number 'component:\s*target' -A 5Length of output: 4633
go.mod (1)
9-10: Verify new logging and configuration dependencies.New dependencies added:
- charmbracelet/lipgloss and log for enhanced logging
- spf13/viper for configuration management
These additions align well with the refactoring goals for improved testing infrastructure.
Also applies to: 15-15
✅ Verification successful
Dependencies are properly integrated and utilized
The new dependencies are correctly implemented:
- Logging (charmbracelet/log) is consistently used across multiple files
- Styling (charmbracelet/lipgloss) is centralized in logger.go
- Configuration (spf13/viper) is properly structured in the config package
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of new dependencies # Search for logging usage echo "Searching for logging implementations..." rg -g '*.go' --line-number 'charmbracelet/(log|lipgloss)' -A 5 # Search for viper configuration usage echo "Searching for viper configuration usage..." rg -g '*.go' --line-number 'spf13/viper' -A 5Length of output: 3502
README.yaml (1)
112-118: LGTM! Documentation improvements.Good improvements to the documentation:
- Package renamed consistently from aws-component-helper to component-helper
- Added helpful reference to the component-helper README
pkg/atmos/component-helper/README.md (1)
1-209: Excellent documentation quality!The README provides comprehensive coverage of:
- Clear overview and goals
- Well-documented example with explanatory comments
- Detailed directory structure
- Thorough explanation of test phases
- Advanced usage scenarios
- Complete flags reference
This will greatly help users understand and utilize the package effectively.
🧰 Tools
🪛 GitHub Actions: Run Tests
[error] Test suite 'TestExampleTestSuite' failed with exit code 1
pkg/atmos/component-helper/dependency/dependency.go (2)
1-2: LGTM! Package name and structure are well-organized.The package name follows Go conventions and clearly indicates its purpose.
🧰 Tools
🪛 GitHub Actions: Run Tests
[error] Test suite 'TestExampleTestSuite' failed with exit code 1
1-7: Verify integration with existing test suites.The failing test suite 'TestExampleTestSuite' might need updates to work with the new
Dependencystructure.✅ Verification successful
Test suite integration verified successfully
The
TestExampleTestSuiteand the newDependencystructure are properly integrated through the baseTestSuiteclass. The test failures are unrelated to the dependency implementation as the example tests only contain basic assertions.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for usages of the old AWS component helper structure echo "Searching for old dependency usage patterns..." rg -l "aws-component-helper" # Find all test files that might need updates echo "Finding test files that might need updates..." fd -e go -x grep -l "TestExampleTestSuite" {}Length of output: 345
Script:
#!/bin/bash # Check the content of the test suite echo "Content of suite_test.go:" cat pkg/atmos/component-helper/suite_test.go # Look for usage of Dependency struct in tests echo -e "\nSearching for Dependency struct usage in tests:" rg "Dependency" pkg/atmos/component-helper/ -A 5 -B 5Length of output: 3516
🧰 Tools
🪛 GitHub Actions: Run Tests
[error] Test suite 'TestExampleTestSuite' failed with exit code 1
mcalhoun
force-pushed
the
refactor-test-helpers
branch
from
71dcb98 to
8e48076
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
go.mod (1)
Line range hint
3-3: Invalid Go version specifiedThe Go version is set to
1.23which is not a valid version. The latest stable version of Go is 1.22.Update the Go version to a valid release:
-go 1.23 +go 1.22
🧹 Nitpick comments (15)
pkg/atmos/component-helper/README.md (5)
5-7: Add reference to Cloud Posse convention documentation.Consider adding a link to documentation explaining the Cloud Posse convention for Components, as this would help new users understand the expected structure and patterns.
56-56: Add error handling example for DeployAtmosComponent.The example ignores the error return value from
DeployAtmosComponent. Consider demonstrating proper error handling:- options, _ := s.DeployAtmosComponent(s.T(), component, stack, nil) + options, err := s.DeployAtmosComponent(s.T(), component, stack, nil) + if err != nil { + s.T().Fatalf("Failed to deploy component: %v", err) + }
97-100: Document configuration for alternative directory structures.You mention that the Helper is configurable for other directory structures, but there's no explanation of how to achieve this. Consider adding examples of common alternative structures and the required configuration.
153-154: Fix typo in fixtures path explanation.There's a typo in "If yyou":
- `fixtures` directory that is being copied, you can use the `--fixtures-path` flag. If yyou want to override the path to the + `fixtures` directory that is being copied, you can use the `--fixtures-path` flag. If you want to override the path to the
190-194: Add common debugging examples.Consider adding examples of common debugging scenarios and the corresponding
atmoscommands that would be helpful when troubleshooting in the temporary directory. This would make the debugging process more straightforward for users.pkg/atmos/component-helper/suite.go (4)
1-2: Consider adding package-level documentation.
Although the package namecomponent_helperis self-explanatory, a brief Go doc comment at the package level would help clarify its purpose to new contributors or maintainers.
22-27: Add doc comment for exported constructor.
Adding a Go doc comment forNewTestSuite()would clarify its purpose and inform devs about the initialization steps (e.g., creation ofDependencies).+// NewTestSuite creates a new TestSuite for Atmos component testing. func NewTestSuite() *TestSuite { tsuite := new(TestSuite) tsuite.Dependencies = make([]*dependency.Dependency, 0) return tsuite }
33-39: Add doc comment forAddDependency.
Public methods can benefit from a doc comment explaining the significance of adding a dependency to the test suite.
80-86: Add doc comment for DestroyAtmosComponent.
Similar toDeployAtmosComponent, this function is exported but lacks input validation and docs.+// DestroyAtmosComponent destroys a deployed Atmos component using merged config func (s *TestSuite) DestroyAtmosComponent(t *testing.T, componentName string, stackName string, additionalVars *map[string]interface{}) { + require.NotEmpty(t, componentName, "componentName cannot be empty") + require.NotEmpty(t, stackName, "stackName cannot be empty") mergedVars := s.getMergedVars(t, additionalVars)pkg/atmos/component-helper/99_destroy.go (1)
13-30: Verify file existence before removal.If the file doesn’t exist, calling
os.Removecan be misleading. Consider checking its existence first or logging a note.pkg/atmos/component-helper/20_enabled_flag_tst.go (1)
9-32: Add assertions for expected behavior when 'enabled' is false.Currently, the code only checks for a successful plan exit code. For robustness, consider verifying that disabling a component leads to the expected plan outcome.
pkg/atmos/component-helper/00_setup.go (2)
15-45: Consider parallelizing directory copying or improving error handling.For large directories, concurrency could speed up copying; partial error handling can skip problematic files while proceeding with others.
47-66: Allow auto-creation of destination directories.Rather than skipping when the destination doesn’t exist, consider creating it to streamline your setup process.
test/component-helper-integration/test/fixtures/stacks/orgs/test/main.yaml (1)
39-53: Components structure is consistent and well-organized.The renaming and addition of components follow a clear pattern with consistent metadata structure. Each component uses the same metadata pattern with sequential revision numbers, which is good for testing scenarios.
Consider using YAML anchors to reduce repetition:
components: terraform: + # Define the base component structure + _component_template: &component_base + metadata: + component: target + vars: {} + component1: - metadata: - component: target + <<: *component_base vars: revision: 1 component2: - metadata: - component: target + <<: *component_base vars: revision: 2 component3: - metadata: - component: target + <<: *component_base vars: revision: 3go.mod (1)
21-25: Consider pinning major versions for Google Cloud dependenciesThe Google Cloud dependencies are using specific patch versions. Consider using major version constraints for better maintenance:
-cloud.google.com/go v0.112.1 // indirect -cloud.google.com/go/compute v1.24.0 // indirect -cloud.google.com/go/iam v1.1.6 // indirect -cloud.google.com/go/storage v1.38.0 // indirect +cloud.google.com/go/compute/v1 v1.24.0 // indirect +cloud.google.com/go/iam/v1 v1.1.6 // indirect +cloud.google.com/go/storage/v1 v1.38.0 // indirect
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (32)
.gitignore(1 hunks)README.md(1 hunks)README.yaml(2 hunks)go.mod(5 hunks)pkg/atmos/aws-component-helper/atmos.go(0 hunks)pkg/atmos/aws-component-helper/cli.go(0 hunks)pkg/atmos/aws-component-helper/cli_test.go(0 hunks)pkg/atmos/aws-component-helper/const.go(0 hunks)pkg/atmos/aws-component-helper/dependency.go(0 hunks)pkg/atmos/aws-component-helper/setup_component.go(0 hunks)pkg/atmos/aws-component-helper/setup_component_test.go(0 hunks)pkg/atmos/aws-component-helper/setup_test_suite.go(0 hunks)pkg/atmos/aws-component-helper/setup_test_suite_test.go(0 hunks)pkg/atmos/aws-component-helper/shared.go(0 hunks)pkg/atmos/aws-component-helper/tear_down_suite.go(0 hunks)pkg/atmos/aws-component-helper/test_suite.go(0 hunks)pkg/atmos/component-helper/00_setup.go(1 hunks)pkg/atmos/component-helper/01_deploy.go(1 hunks)pkg/atmos/component-helper/10_vendor.go(1 hunks)pkg/atmos/component-helper/20_enabled_flag_tst.go(1 hunks)pkg/atmos/component-helper/99_destroy.go(1 hunks)pkg/atmos/component-helper/README.md(1 hunks)pkg/atmos/component-helper/atmos_options.go(1 hunks)pkg/atmos/component-helper/bools.go(1 hunks)pkg/atmos/component-helper/config/config.go(1 hunks)pkg/atmos/component-helper/dependency/dependency.go(1 hunks)pkg/atmos/component-helper/logger.go(1 hunks)pkg/atmos/component-helper/suite.go(1 hunks)test/aws-component-helper/test/basic_test.go(0 hunks)test/component-helper-integration/test/component_helper_integration_test.go(1 hunks)test/component-helper-integration/test/fixtures/stacks/orgs/test/main.yaml(2 hunks)test/component-helper-integration/test/fixtures/vendor.yaml(1 hunks)
💤 Files with no reviewable changes (13)
- pkg/atmos/aws-component-helper/const.go
- pkg/atmos/aws-component-helper/dependency.go
- pkg/atmos/aws-component-helper/tear_down_suite.go
- test/aws-component-helper/test/basic_test.go
- pkg/atmos/aws-component-helper/setup_component.go
- pkg/atmos/aws-component-helper/setup_component_test.go
- pkg/atmos/aws-component-helper/shared.go
- pkg/atmos/aws-component-helper/setup_test_suite_test.go
- pkg/atmos/aws-component-helper/cli.go
- pkg/atmos/aws-component-helper/atmos.go
- pkg/atmos/aws-component-helper/cli_test.go
- pkg/atmos/aws-component-helper/setup_test_suite.go
- pkg/atmos/aws-component-helper/test_suite.go
🚧 Files skipped from review as they are similar to previous changes (12)
- README.md
- .gitignore
- README.yaml
- pkg/atmos/component-helper/01_deploy.go
- pkg/atmos/component-helper/atmos_options.go
- test/component-helper-integration/test/fixtures/vendor.yaml
- pkg/atmos/component-helper/bools.go
- pkg/atmos/component-helper/dependency/dependency.go
- pkg/atmos/component-helper/logger.go
- pkg/atmos/component-helper/config/config.go
- test/component-helper-integration/test/component_helper_integration_test.go
- pkg/atmos/component-helper/10_vendor.go
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Run Tests
🔇 Additional comments (19)
pkg/atmos/component-helper/suite.go (7)
3-14: Evaluate unused imports and align with the refactor objective.
The import list is concise, but if concurrency or advanced logging is planned, consider adding relevant libraries to support future improvements without clutter.
16-20: Well-structured embedding of suite.Suite.
Embeddingsuite.Suitein theTestSuitestruct is a well-known approach that simplifies test organization. This is aligned with the refactoring objective of making the test framework more accessible.
29-31: Confirm concurrency handling if multiple test suites run in parallel.
Run()leveragessuite.Run, which can be parallelized. Ensure configuration ins.Configis safe for concurrent usage.
41-54: Validate merging logic for conflicting keys.
getMergedVarsmerges additional vars into a baseline map. If there's a risk of collisions, consider logging or clarifying the priority of conflicts in the doc.
56-78: Add input validation and documentation for DeployAtmosComponent.
This comment matches a past suggestion regarding input validation (componentName, stackName) and doc strings for an exported function.+// DeployAtmosComponent deploys an Atmos component with the specified configuration +// and merges additionalVars on top of the default ones. func (s *TestSuite) DeployAtmosComponent(t *testing.T, componentName string, stackName string, additionalVars *map[string]interface{}) (*atmos.Options, string) { + require.NotEmpty(t, componentName, "componentName cannot be empty") + require.NotEmpty(t, stackName, "stackName cannot be empty") phaseName := fmt.Sprintf("deploy/atmos component/%s/%s", stackName, componentName) ...
88-106: Fix indentation and improve error handling in SetupSuite.
This mirrors a past recommendation to fix the improper indentation at line 100 and add error checks if necessary.
108-121: Enhance teardown clarity.
The teardown methods look well-sequenced and consistent with typical testing patterns. Consider adding doc comments for future maintainers, explaining the significance of each teardown step (likeDestroyTempDirorDestroyConfigFile).pkg/atmos/component-helper/99_destroy.go (2)
32-60: Improve error handling to ensure all dependencies are processed.This matches a past review comment about collecting errors and continuing the destroy loop.
62-81: Ensure both directories are removed regardless of partial failures.This matches a past review comment about collecting errors when removing multiple directories.
pkg/atmos/component-helper/00_setup.go (5)
68-81: LGTM.Temp directory setup is straightforward and correct.
83-97: LGTM.State directory initialization is clear and well-handled.
99-109: LGTM.Paths for Atmos components and stacks are set up correctly.
111-122: LGTM.Bootstrap process seems robust and thorough.
124-139: Fix directory validation and control flow in CopyComponentToTempDir.This matches a past review comment recommending verification of source directory existence before copying.
test/component-helper-integration/test/fixtures/stacks/orgs/test/main.yaml (2)
6-6: LGTM! Region variable addition is well-structured.The addition of the
regionvariable follows the standard configuration pattern and uses a common AWS testing region.
39-53: Verify component references in test code.Let's ensure these components are properly referenced in the test code and that no old references to
dep1remain.✅ Verification successful
Components are properly referenced in test code ✅
All three components are actively used in the test suite with proper test helper methods:
component1andcomponent3as dependencies in test setupcomponent2in test cases for enabled flag and component deployment🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for both old (dep1) and new (component1,2,3) component references echo "Checking for old 'dep1' references..." rg -l "dep1" --type go echo -e "\nChecking for new component references..." rg -l "component[123]" --type go echo -e "\nChecking for potential test files using these components..." fd -e go -e yaml -e tf "test" | xargs rg -l "component[123]"Length of output: 593
Script:
#!/bin/bash # Check the context of component usage in the test file echo "Examining component usage in test file:" rg -A 5 -B 5 "component[123]" test/component-helper-integration/test/component_helper_integration_test.goLength of output: 852
go.mod (3)
123-126: Update Google API dependencies as a groupThe Google API dependencies (genproto, grpc) should be updated together to ensure compatibility:
✅ Verification successful
✓ Google API dependencies are properly aligned
The Google API dependencies are at their latest compatible versions from March 2024, with grpc at v1.62.1 and genproto packages updated together between March 13-14, 2024. No version mismatches found.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for potential version mismatches in Google APIs echo "Checking go.sum for Google API versions..." rg "google.golang.org/(genproto|grpc|protobuf)" go.sum | sortLength of output: 19123
9-10: Verify the necessity of new UI and logging dependenciesThe addition of
charmbracelet/lipgloss,charmbracelet/log, andspf13/vipersignificantly increases the dependency footprint. While these are high-quality libraries, they should be justified by the refactoring requirements.Also applies to: 15-15
✅ Verification successful
Dependencies are well-justified and appropriately utilized
The new dependencies serve clear purposes:
lipglossprovides consistent styled console output for better user experiencecharmbracelet/logimplements structured logging across all component phasesvipermanages complex configuration with support for both file and flag-based settings🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for usage of new UI and logging libraries echo "Searching for charmbracelet/lipgloss usage..." rg "lipgloss" --type go echo -e "\nSearching for charmbracelet/log usage..." rg "charmbracelet/log" --type go echo -e "\nSearching for viper config usage..." rg "viper" --type goLength of output: 5537
109-113: Verify OpenTelemetry integration requirementsNew OpenTelemetry dependencies have been added. These are monitoring and observability tools that require proper setup and configuration.
mcalhoun
force-pushed
the
refactor-test-helpers
branch
3 times, most recently
from
a2878e5 to
be886b5
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
go.mod (1)
Line range hint
3-3: Invalid Go version specifiedThe Go version
1.23specified in thego.modfile is not yet released. The latest stable version is Go 1.22.Update the Go version:
-go 1.23 +go 1.22
🧹 Nitpick comments (5)
pkg/atmos/component-helper/README.md (3)
38-41: Fix typo in comment.There's a typo in the comment: "out" should be "our".
-// Next we can define a set of tests that will be run for out component. These tests are defined as methods on the suite +// Next we can define a set of tests that will be run for our component. These tests are defined as methods on the suite
97-99: Fix duplicate word in sentence.Remove the duplicate "are" in the sentence.
-configuration, and tests are are located within a directory that is structured as described below, however, The Helper +configuration, and tests are located within a directory that is structured as described below, however, The Helper
153-154: Fix typo in setup phase description.There's a typo in the word "yyou".
-`fixtures` directory that is being copied, you can use the `--fixtures-path` flag. If yyou want to override the path to the +`fixtures` directory that is being copied, you can use the `--fixtures-path` flag. If you want to override the path to thepkg/atmos/component-helper/config/config.go (1)
71-79: Consider refactoring theisPFlagBoundcheck.
Using string prefixes ("skip"and"test") and hard-coded checks likekey == "configfilepath"can be brittle. If additional flags are introduced or naming patterns change, this logic can break silently. Consider a more robust mechanism (e.g., maintaining a registry of bound keys).go.mod (1)
9-10: Architectural change: New logging and configuration managementThe addition of
charmbracelet/lipgloss,charmbracelet/log, andspf13/vipersuggests significant changes to logging and configuration management. These are robust choices:
charmbraceletprovides modern, structured logging with good terminal supportviperis a complete configuration solution supporting multiple formats and dynamic reloadingPlease ensure:
- Migration guide is provided for users updating from the old logging system
- Documentation is updated to reflect new configuration capabilities
- Examples are provided for common use cases
Also applies to: 15-15
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (32)
.gitignore(1 hunks)README.md(3 hunks)README.yaml(2 hunks)go.mod(5 hunks)pkg/atmos/aws-component-helper/atmos.go(0 hunks)pkg/atmos/aws-component-helper/cli.go(0 hunks)pkg/atmos/aws-component-helper/cli_test.go(0 hunks)pkg/atmos/aws-component-helper/const.go(0 hunks)pkg/atmos/aws-component-helper/dependency.go(0 hunks)pkg/atmos/aws-component-helper/setup_component.go(0 hunks)pkg/atmos/aws-component-helper/setup_component_test.go(0 hunks)pkg/atmos/aws-component-helper/setup_test_suite.go(0 hunks)pkg/atmos/aws-component-helper/setup_test_suite_test.go(0 hunks)pkg/atmos/aws-component-helper/shared.go(0 hunks)pkg/atmos/aws-component-helper/tear_down_suite.go(0 hunks)pkg/atmos/aws-component-helper/test_suite.go(0 hunks)pkg/atmos/component-helper/00_setup.go(1 hunks)pkg/atmos/component-helper/01_deploy.go(1 hunks)pkg/atmos/component-helper/10_vendor.go(1 hunks)pkg/atmos/component-helper/20_enabled_flag_tst.go(1 hunks)pkg/atmos/component-helper/99_destroy.go(1 hunks)pkg/atmos/component-helper/README.md(1 hunks)pkg/atmos/component-helper/atmos_options.go(1 hunks)pkg/atmos/component-helper/bools.go(1 hunks)pkg/atmos/component-helper/config/config.go(1 hunks)pkg/atmos/component-helper/dependency/dependency.go(1 hunks)pkg/atmos/component-helper/logger.go(1 hunks)pkg/atmos/component-helper/suite.go(1 hunks)test/aws-component-helper/test/basic_test.go(0 hunks)test/component-helper-integration/test/component_helper_integration_test.go(1 hunks)test/component-helper-integration/test/fixtures/stacks/orgs/test/main.yaml(2 hunks)test/component-helper-integration/test/fixtures/vendor.yaml(1 hunks)
💤 Files with no reviewable changes (13)
- pkg/atmos/aws-component-helper/const.go
- pkg/atmos/aws-component-helper/dependency.go
- pkg/atmos/aws-component-helper/shared.go
- pkg/atmos/aws-component-helper/setup_component_test.go
- pkg/atmos/aws-component-helper/setup_component.go
- test/aws-component-helper/test/basic_test.go
- pkg/atmos/aws-component-helper/setup_test_suite_test.go
- pkg/atmos/aws-component-helper/setup_test_suite.go
- pkg/atmos/aws-component-helper/cli.go
- pkg/atmos/aws-component-helper/cli_test.go
- pkg/atmos/aws-component-helper/tear_down_suite.go
- pkg/atmos/aws-component-helper/atmos.go
- pkg/atmos/aws-component-helper/test_suite.go
🚧 Files skipped from review as they are similar to previous changes (14)
- .gitignore
- test/component-helper-integration/test/fixtures/vendor.yaml
- pkg/atmos/component-helper/dependency/dependency.go
- README.md
- pkg/atmos/component-helper/20_enabled_flag_tst.go
- pkg/atmos/component-helper/01_deploy.go
- test/component-helper-integration/test/fixtures/stacks/orgs/test/main.yaml
- pkg/atmos/component-helper/10_vendor.go
- pkg/atmos/component-helper/bools.go
- pkg/atmos/component-helper/logger.go
- pkg/atmos/component-helper/atmos_options.go
- README.yaml
- test/component-helper-integration/test/component_helper_integration_test.go
- pkg/atmos/component-helper/suite.go
🧰 Additional context used
📓 Learnings (1)
pkg/atmos/component-helper/config/config.go (1)
Learnt from: mcalhoun
PR: cloudposse/test-helpers#44
File: pkg/atmos/component-helper/config/config.go:17-24
Timestamp: 2025-01-28T18:30:11.265Z
Learning: In the component-helper package, skip flags (e.g., skip-deploy-component, skip-deploy-dependencies) are set to `true` by default when explicitly used because skipping test phases is the intended behavior in those cases. These defaults do not affect the normal test execution when the flags are not used.
🔇 Additional comments (8)
pkg/atmos/component-helper/README.md (2)
197-209: Add missing --fixtures-path flag to reference table.The
--fixtures-pathflag is mentioned in the Test Phases section but is missing from the flags reference table.Add the following row to the flags table:
| -src-dir | The path to the component source directory | src/ | +| -fixtures-path | The path to the fixtures directory | test/fixtures/ | | -state-dir | The path to the terraform state directory | {temp_dir}/state |
56-56: Document error handling for DeployAtmosComponent.The example ignores the error return value from
DeployAtmosComponent. Consider documenting proper error handling.- options, _ := s.DeployAtmosComponent(s.T(), component, stack, nil) + options, err := s.DeployAtmosComponent(s.T(), component, stack, nil) + if err != nil { + s.T().Fatalf("Failed to deploy component: %v", err) + }pkg/atmos/component-helper/config/config.go (2)
17-24: Default skip flags logic appears intentional.
Based on prior discussions and learnings, setting skip flags default totrueis the intended behavior, as these flags only come into effect when explicitly used. No changes needed here.
100-100: Revisit default value forSrcDir.
The default../srcmay fail in certain execution contexts if this relative directory does not exist. This was flagged previously.pkg/atmos/component-helper/99_destroy.go (1)
72-79: Fail-fast approach for directory removal is acceptable.
This code ensures any error stops the process immediately, preventing partial or stale cleanup. Marking as good given the intentional design choice.pkg/atmos/component-helper/00_setup.go (2)
15-45: Code for recursively copying directories is clear and well-structured.
The logic usesfilepath.Walkand handles file/directory creation appropriately.
124-139: Consider skipping the copy earlier ifSkipSetupTestSuiteis set.
Currently, the function always copies before checking the skip flag at the end. A prior review comment addressed similar concerns regarding directory validation and control flow.go.mod (1)
17-17: LGTM: Dependency updates look goodThe updates to core dependencies are appropriate and include important security fixes:
golang.org/x/expupdated to latest- Cloud SDK components updated to latest versions
- gRPC and related Google APIs updated to latest versions
Also applies to: 21-25, 61-61, 75-75, 116-116, 123-126
mcalhoun
force-pushed
the
refactor-test-helpers
branch
from
be886b5 to
4ef2513
Compare
mcalhoun
force-pushed
the
refactor-test-helpers
branch
from
4ef2513 to
6ba16a1
Compare
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
Actionable comments posted: 2
♻️ Duplicate comments (2)
pkg/atmos/component-helper/README.md (1)
197-210:⚠️ Potential issueFix inconsistencies in flag documentation.
- The flag is referred to as
--fixtures-pathin the text but listed as-fixtures-dirin the table.- The
--fixtures-pathflag mentioned in the Test Phases section is missing from the flags reference table.Please standardize the flag naming and ensure all flags are documented.
| Flag | Description | Default | | -------------------------- | ----------------------------------------------------- | ----------------- | | -config | The path to the config file | test_suite.yaml | -| -fixtures-dir | The path to the fixtures directory | fixtures | +| -fixtures-path | The path to the fixtures directory | fixtures |pkg/atmos/component-helper/00_setup.go (1)
124-139:⚠️ Potential issueFix directory validation and control flow in CopyComponentToTempDir.
The issues mentioned in the past review comment are still present and need to be addressed.
Apply this diff to fix the issues:
func (s *TestSuite) CopyComponentToTempDir(t *testing.T, config *c.Config) { + if s.Config.SkipSetupTestSuite { + s.logPhaseStatus("setup/copy component to temp dir", "skipped") + return + } + s.logPhaseStatus("setup/copy component to temp dir", "started") + // Verify source directory exists + if _, err := os.Stat(config.SrcDir); os.IsNotExist(err) { + s.logPhaseStatus("setup/copy component to temp dir", "failed") + require.NoError(t, fmt.Errorf("source directory does not exist: %s", config.SrcDir)) + return + } + destPath := filepath.Join(config.TempDir, "components", "terraform", "target") err := s.copyDirectoryContents(config.SrcDir, destPath) if err != nil { s.logPhaseStatus("setup/copy component to temp dir", "failed") require.NoError(t, err) } s.logPhaseStatus("setup/copy component to temp dir", "completed") - - if s.Config.SkipSetupTestSuite { - return - } }
🧹 Nitpick comments (4)
pkg/atmos/component-helper/README.md (4)
38-41: Fix typo in comment.There's a typo in the comment: "out" should be "our".
-// Next we can define a set of tests that will be run for out component. These tests are defined as methods on the suite +// Next we can define a set of tests that will be run for our component. These tests are defined as methods on the suite
58-62: Consider making the example more generic.The comment references a specific output
vpc_cidrwhich might be confusing for users testing different components. Consider using a more generic example or adding a note that this is specific to VPC components.- // Finally, as an example, we can use the atmos Output() method to get the value of the `vpc_cidr` output from the + // Finally, as an example, we can use the atmos Output() method to get the value of an output from the // component and assert that it is equal to the expected value. - cidrBlock := atmos.Output(s.T(), options, "vpc_cidr") - assert.Equal(s.T(), "10.1.0.0/16", cidrBlock) + // For a VPC component, we might check the CIDR block: + cidrBlock := atmos.Output(s.T(), options, "vpc_cidr") + assert.Equal(s.T(), "10.1.0.0/16", cidrBlock)
97-99: Fix duplicate word in sentence.There's a duplicate word "are" in the sentence.
-The Helper assumes that each test suite is used to test a component (root module) and that the component, atmos stack -configuration, and tests are are located within a directory that is structured as described below, however, The Helper +The Helper assumes that each test suite is used to test a component (root module) and that the component, atmos stack +configuration, and tests are located within a directory that is structured as described below, however, The Helper
153-155: Fix typo in sentence.There's a typo: "yyou" should be "you".
-`fixtures` directory, the `atmos.yaml` file, and the `vendor.yaml` file. If yyou want to override the path to the +`fixtures` directory, the `atmos.yaml` file, and the `vendor.yaml` file. If you want to override the path to the
🛑 Comments failed to post (2)
pkg/atmos/component-helper/00_setup.go (2)
47-66: 🛠️ Refactor suggestion
Fix potential race condition in copyDirectoryRecursively.
The function has a Time-of-check to time-of-use (TOCTOU) race condition between directory existence checks and operations.
Apply this diff to improve the function:
func (s *TestSuite) copyDirectoryRecursively(t *testing.T, srcDir string, destDir string) { - _, err := os.Stat(srcDir) - if os.IsNotExist(err) { + if _, err := os.Stat(srcDir); err != nil { path := fmt.Sprintf("setup/copy %s to %s, source directory does not exist", srcDir, destDir) s.logPhaseStatus(path, "skipped") return } - _, err = os.Stat(destDir) - if os.IsNotExist(err) { - path := fmt.Sprintf("setup/copy %s to %s, destination directory does not exist", srcDir, destDir) - s.logPhaseStatus(path, "skipped") - return - } - log.Debug("copying directory recursively", "srcDir", srcDir, "destDir", destDir) - err = s.copyDirectoryContents(srcDir, destDir) + err := s.copyDirectoryContents(srcDir, destDir) require.NoError(t, 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.func (s *TestSuite) copyDirectoryRecursively(t *testing.T, srcDir string, destDir string) { if _, err := os.Stat(srcDir); err != nil { path := fmt.Sprintf("setup/copy %s to %s, source directory does not exist", srcDir, destDir) s.logPhaseStatus(path, "skipped") return } log.Debug("copying directory recursively", "srcDir", srcDir, "destDir", destDir) err := s.copyDirectoryContents(srcDir, destDir) require.NoError(t, err) }
15-45: 🛠️ Refactor suggestion
Add parent directory creation and symlink handling in copyDirectoryContents.
The function needs to handle several edge cases:
- Parent directories should be created before writing files
- Symlinks should be handled appropriately
Apply this diff to improve the function:
func (s *TestSuite) copyDirectoryContents(srcDir string, destDir string) error { + // Create destination directory if it doesn't exist + if err := os.MkdirAll(destDir, 0755); err != nil { + return err + } + // Walk through all files and directories in srcDir and copy them to destDir return filepath.Walk(srcDir, func(path string, info os.FileInfo, err error) error { if err != nil { return err } // Calculate the relative path from srcDir relPath, err := filepath.Rel(srcDir, path) if err != nil { return err } // Calculate destination path in destDir destPath := filepath.Join(destDir, relPath) + // Handle symlinks + if info.Mode()&os.ModeSymlink != 0 { + linkTarget, err := os.Readlink(path) + if err != nil { + return err + } + return os.Symlink(linkTarget, destPath) + } + if info.IsDir() { // Create directory in destination return os.MkdirAll(destPath, info.Mode()) } else { + // Ensure parent directory exists + if err := os.MkdirAll(filepath.Dir(destPath), 0755); err != nil { + return err + } + // Copy file content content, err := os.ReadFile(path) if err != nil { return err } return os.WriteFile(destPath, content, info.Mode()) } }) }📝 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.func (s *TestSuite) copyDirectoryContents(srcDir string, destDir string) error { // Create destination directory if it doesn't exist if err := os.MkdirAll(destDir, 0755); err != nil { return err } // Walk through all files and directories in srcDir and copy them to destDir return filepath.Walk(srcDir, func(path string, info os.FileInfo, err error) error { if err != nil { return err } // Calculate the relative path from srcDir relPath, err := filepath.Rel(srcDir, path) if err != nil { return err } // Calculate destination path in destDir destPath := filepath.Join(destDir, relPath) // Handle symlinks if info.Mode()&os.ModeSymlink != 0 { linkTarget, err := os.Readlink(path) if err != nil { return err } return os.Symlink(linkTarget, destPath) } if info.IsDir() { // Create directory in destination return os.MkdirAll(destPath, info.Mode()) } else { // Ensure parent directory exists if err := os.MkdirAll(filepath.Dir(destPath), 0755); err != nil { return err } // Copy file content content, err := os.ReadFile(path) if err != nil { return err } return os.WriteFile(destPath, content, info.Mode()) } }) }
what
atmos/component-helperpackage to better handle running suites of component testswhy
After multiple attempts at creating a set of helpers for running
atmoscomponent tests while reducing the amount of boilerplate code we needed to write and balancing ease of use (simple API) for new Go developers, we arrived at the current version of the helpers. The previous version still required significant code, relied on TestMain (which although idiomatic, still was confusing for newer Go devs) and didn't handle concurrency well. This updated version should address those issues.references