feat: implement precondition-based test skipping for better developer experience #1450
+1,268
−14
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
… experience This commit introduces a comprehensive testing strategy that gracefully handles missing environmental preconditions, making Atmos locally testable for developers without requiring full environment setup. ## Changes ### Core Implementation - Added `tests/test_preconditions.go` with SDK-based helper functions for checking: - AWS profile availability (`RequireAWSProfile`) - Git repository and remote configuration (`RequireGitRepository`, `RequireGitRemoteWithValidURL`) - GitHub network access and rate limits (`RequireGitHubAccess`) - OCI registry authentication via GitHub token (`RequireOCIAuthentication`) - General network connectivity (`RequireNetworkAccess`) - Environment variables, executables, and file paths ### Test Updates - Updated AWS-related tests to check for AWS profile availability - Updated Git-related tests to verify repository and remote configuration - Updated vendor tests to check for GitHub access and OCI authentication - All tests now skip gracefully with informative messages when preconditions aren't met ### Documentation - Created comprehensive PRD at `docs/prd/testing-strategy.md` outlining the testing strategy - Updated `tests/README.md` with practical guidance and examples - Updated `CLAUDE.md` with references to new testing approach ### Environment Variables - Added `ATMOS_TEST_SKIP_PRECONDITION_CHECKS=true` to bypass all precondition checks - Supports both `GITHUB_TOKEN` and `ATMOS_GITHUB_TOKEN` for OCI authentication ## Benefits - Tests no longer fail due to missing environment setup - Clear, actionable skip messages guide developers to fix issues - CI/CD can bypass checks when running with mocked dependencies - Improves developer onboarding and contribution experience 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
osterman
added
the
no-release
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1450 +/- ##
==========================================
+ Coverage 55.94% 56.15% +0.21%
==========================================
Files 274 275 +1
Lines 28936 29150 +214
==========================================
+ Hits 16188 16369 +181
- Misses 10956 10972 +16
- Partials 1792 1809 +17
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
- Added test_preconditions_test.go with comprehensive tests for all helper functions - Achieved 41.2% coverage (up from 36.81%) for the tests package - Updated .golangci.yml with text-based exclusion rules for os.Getenv/Setenv in test files - Kept nolint:forbidigo comments as text-based exclusion didn't work reliably - Updated GitHub Actions to use golangci-lint-action@v8 (latest version) - All precondition helpers now have tests covering bypass scenarios and actual usage The test coverage improvement helps meet the minimum testing requirements for the PR.
osterman
force-pushed
the
skip-tests-missing-precondition
branch
from
b705af5 to
9fa9407
Compare
github-actions
bot
added
size/xl
|
Warning This PR exceeds the recommended limit of 1,000 lines.Large PRs are difficult to review and may be rejected due to their size. Please verify that this PR does not address multiple issues. |
|
Important Cloud Posse Engineering Team Review RequiredThis pull request modifies files that require Cloud Posse's review. Please be patient, and a core maintainer will review your changes. To expedite this process, reach out to us on Slack in the |
Remove the os.Setenv pattern from forbidigo linter configuration as requested. Keep os.Getenv pattern for production code but use nolint comments in test files. After extensive sandbox testing, determined that text-based exclusion patterns don't work reliably with forbidigo in golangci-lint. The most reliable approach is using //nolint:forbidigo comments in test files where os.Getenv is legitimately needed. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
…ns, not entire testing strategy - Renamed testing-strategy.md to test-preconditions.md to accurately reflect content - Rewrote document to position precondition checking as one component of testing strategy - Removed claims about defining the entire testing approach - Added context about relationship to broader testing framework - Focused content on the specific capability of handling missing environmental dependencies 🤖 Generated with Claude Code Co-Authored-By: Claude <[email protected]>
- Added tests for RequireAWSProfile with non-existent profile (94.1% coverage) - Added tests for RequireGitRepository in and out of repo (85.7% coverage) - Added tests for RequireGitHubAccess without token (72.4% coverage) - Added tests for RequireNetworkAccess with valid/invalid URLs (90.0% coverage) - Added tests for RequireGitRemoteWithValidURL scenarios - Overall test coverage increased from 41.2% to 78.1%, exceeding 60% target 🤖 Generated with Claude Code Co-Authored-By: Claude <[email protected]>
- Removed unsupported 'exclude-rules' field that caused CI failure - Restored 'exclusions' structure compatible with golangci-lint v2.4.0 - Forbidigo is now properly excluded from test files via exclusions.rules - Kept os.Setenv pattern removed as requested - Fixed pattern syntax for os.Getenv (removed unnecessary quotes) 🤖 Generated with Claude Code Co-Authored-By: Claude <[email protected]>
There was a problem hiding this comment.
golangci-lint found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.
- Add periods to all test function comments (godot) - Replace magic numbers with named constants - Refactor RequireGitHubAccess to split rate limit logic into separate function - Fix function-length issue by extracting checkGitHubRateLimit helper These changes address all critical linting errors reported by CI.
- Extract setAWSProfileEnv helper function to reduce nesting complexity - Simplifies AWS profile environment variable management - Reduces cognitive complexity from 4 to acceptable levels
- Add envAWSProfile constant to avoid string literal repetition - Replace all AWS_PROFILE string literals with the constant - Addresses linting warning about repeated string literals
- Add section on how to check PR checks status - Include commands for getting check run annotations - Add examples for inspecting code scanning alerts - Provide concrete examples for the Atmos repository
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
The addition of test precondition checks was causing tests to be skipped in CI environments where AWS credentials and GitHub tokens weren't available. This resulted in a 1.84% decrease in code coverage. By setting ATMOS_TEST_SKIP_PRECONDITION_CHECKS=true in the GitHub Actions workflow, tests will bypass precondition checks and run normally in CI, maintaining the expected coverage levels while still providing helpful skip messages for developers running tests locally. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Problem
Previously, ~18 tests were failing locally due to missing environmental setup (AWS profiles, Git configuration, network access, GitHub tokens), making it difficult for developers to contribute to Atmos without extensive environment configuration.
Solution
This PR introduces intelligent precondition checking that:
ATMOS_TEST_SKIP_PRECONDITION_CHECKS=truefor CI/CD environmentsChanges
New Files
tests/test_preconditions.go- Centralized helper functions for precondition checkingdocs/prd/testing-strategy.md- Product Requirements Document for the testing strategytests/README.md- Practical developer guidanceHelper Functions Added
RequireAWSProfile(t, profile)- Validates AWS profile configurationRequireGitRepository(t)- Checks for Git repositoryRequireGitRemoteWithValidURL(t)- Validates Git remote URLsRequireGitHubAccess(t)- Checks GitHub connectivity and rate limitsRequireOCIAuthentication(t)- Validates GitHub token for OCI registry accessRequireNetworkAccess(t, url)- General network connectivity checkTests Updated
internal/aws_utils,internal/terraform_backend,pkg/storeinternal/exec/describe_affected,internal/exec/terraform_utils,pkg/atlantis,pkg/describeinternal/exec/vendor_utils(GitHub + OCI auth)Testing
All previously failing packages now pass:
Tests skip with helpful messages when preconditions aren't met:
Benefits
✅ Developers can run tests locally without extensive environment setup
✅ Clear skip messages guide developers to fix specific issues
✅ CI/CD can bypass checks when using mocked dependencies
✅ Improves contributor onboarding experience
✅ Distinguishes between environmental issues and actual code failures
References
t.Skipf()🤖 Generated with Claude Code