Reorganise the CLI structure #16
Merged
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
Contributor
|
Oh, yes, thank you! I was thinking along the same lines when I was adding tests. Thanks for this cleanup! |
Owner
Author
|
Thanks for the input! |
kradalby
referenced
this pull request
in kradalby/headscale
Sep 17, 2025
# This is the 1st commit message: integration/auth: increase NetInfo validation timeout to 3 minutes Increase requireAllClientsNetInfoAndDERP timeout from 1 minute to 3 minutes to handle slower NetInfo propagation in CI environments. Add explicit WaitForTailscaleSync before NetInfo validation after reconnection to ensure proper state propagation. Increase polling interval to 5 seconds to reduce system load during validation. # This is the commit message #2: integration/auth: add missing validation to OIDC relogin test Add requireAllClientsOnline and requireAllClientsNetInfoAndDERP validation calls to TestOIDCReloginSameNodeNewUser to match the pattern used in TestAuthKeyLogoutAndReloginSameUser. Validate connection state after initial login, after user2 login, and after logging back into user1. # This is the commit message #3: integration/auth: add missing validation to web flow relogin test Add requireAllClientsOnline and requireAllClientsNetInfoAndDERP validation calls to TestAuthWebFlowLogoutAndRelogin to ensure proper state validation during logout/relogin cycles. Validate initial connection, logout state, and relogin state. # This is the commit message #4: integration/auth: add missing validation to auth key relogin tests Add requireAllClientsOnline and requireAllClientsNetInfoAndDERP validation calls to TestAuthKeyLogoutAndReloginNewUser and TestAuthKeyLogoutAndReloginSameUserExpiredKey to ensure proper state validation during logout/relogin cycles. # This is the commit message #5: integration/helpers: extract expectedNodes collection helper Create collectExpectedNodeIDs helper function to reduce code duplication across relogin tests when collecting node IDs from client status. Replace inline node ID collection in auth key and web flow tests. # This is the commit message #6: integration/auth: fix EventuallyWithT patterns in relogin tests Wrap all external calls (headscale.ListNodes, ts.Status) in EventuallyWithT with appropriate timeouts to handle eventual consistency. Increase timeouts for node count validation to handle cleanup timing and add descriptive messages for debugging. Use longer timeouts for node cleanup scenarios. # This is the commit message #7: integration/auth: standardize timeouts across relogin tests Use consistent timeout values: 120s for initial connection and relogin validation, 120s for logout validation, 180s (3min) for NetInfo/DERP validation to handle CI environment timing variations. # This is the commit message #8: integration/helpers: consolidate collectExpectedNodeIDs helper Move collectExpectedNodeIDs helper function to general_test.go to avoid redeclaration errors and make it available to all integration tests. Remove duplicate implementations from individual test files. # This is the commit message #9: integration/auth: improve node cleanup and add helper functions Fix node cleanup timing in OIDC relogin test by adding explicit node count validation before user switches to prevent "3 nodes instead of 2" failures. Add validateInitialConnection, validateLogoutComplete, and validateReloginComplete helper functions to reduce code duplication and improve maintainability. # This is the commit message #10: integration/auth: add comprehensive documentation to relogin tests Add detailed test documentation explaining the relogin scenarios and use helper functions consistently across OIDC test to improve code clarity and maintainability. # This is the commit message #11: integration: consolidate all helper functions into helpers.go Create comprehensive helpers.go file containing all non-test helper functions: - Move all functions from utils.go - Consolidate helper functions from auth_key_test.go, auth_oidc_test.go, general_test.go - Remove duplicated functions and maintain single source of truth - Organize into logical sections: assertions, node collection, validation, network testing, client state validation, command execution, and policy helpers This creates a single location for all integration test helper functions, improving maintainability and reducing code duplication. # This is the commit message #12: integration: consolidate all helper functions into helpers.go Create comprehensive helpers.go file containing all non-test helper functions: - Move all functions from utils.go - Consolidate helper functions from auth_key_test.go, auth_oidc_test.go, general_test.go - Remove duplicated functions and maintain single source of truth - Organize into logical sections: assertions, node collection, validation, network testing, client state validation, command execution, and policy helpers This creates a single location for all integration test helper functions, improving maintainability and reducing code duplication. # This is the commit message #13: integration: consolidate all helper functions into helpers.go Create comprehensive helpers.go file containing all non-test helper functions: - Move all functions from utils.go - Consolidate helper functions from auth_key_test.go, auth_oidc_test.go, general_test.go - Remove duplicated functions and maintain single source of truth - Organize into logical sections: assertions, node collection, validation, network testing, client state validation, command execution, and policy helpers This creates a single location for all integration test helper functions, improving maintainability and reducing code duplication. # This is the commit message #14: integration/helpers: remove unnecessary basic require helpers Remove basic wrapper functions like requireNoErr, requireNoErrHeadscaleEnv, etc. since they just wrap standard require.NoError calls. Tests should use require.NoError, require.NoErrorf, require.Contains, etc. directly instead of these unnecessary helper wrappers. The remaining helper functions in helpers.go are now focused on actual integration test logic rather than simple assertion wrappers. # This is the commit message #15: integration/helpers: restore basic require helpers to fix compilation Revert the removal of basic require helpers since they are still used throughout the integration tests. The proper approach is to: 1. Keep the helpers for now to maintain compilation 2. Gradually replace usage with direct require.NoError calls 3. Remove helpers only after all usage is updated This maintains a working codebase while allowing incremental improvement. # This is the commit message #16: integration: remove unnecessary require wrapper functions Remove requireNoErr, requireNoErrf, and requireContains wrapper functions and replace their usage with direct require.NoError, require.NoErrorf, and require.Contains calls. These wrappers provided no value over the standard testify require functions. - Updated 154 requireNoErr calls to require.NoError - Updated 5 requireNoErrf calls to require.NoErrorf with correct argument order - Updated 1 requireContains call to require.Contains - Added require import to test files that needed it The remaining helpers in helpers.go now focus on actual integration test logic rather than unnecessary assertion wrappers. # This is the commit message juanfont#17: integration/helpers: remove section comment headers Remove all // ===== section comment headers as they add visual noise without providing value. The functions are logically grouped and have proper documentation comments that explain their purpose. # This is the commit message juanfont#18: integration: add comprehensive godoc to all helper functions - Add godoc comments to all previously undocumented functions in helpers.go - Document validation purpose and failure context for assertion helpers - Explain network testing helpers and their specific use cases - Document policy v2 configuration builders and OIDC test utilities - Improve code maintainability and developer onboarding experience Fixes missing documentation identified in integration test helper analysis. # This is the commit message juanfont#19: integration/auth: fix node cleanup timing in OIDC relogin test - Add explicit node count validation with proper timing for cleanup - Wait for node creation before checking for exact count (avoid 3 nodes issue) - Extend timeout to 90s for node cleanup stabilization in CI environments - Add node stability validation to ensure no phantom nodes remain - Add proper logout validation before relogin to ensure clean state - Improve error handling during login process to handle temporary instability Addresses Phase 3.1 of relogin test flakiness plan - fixes the '3 nodes instead of 2' failure by properly handling cleanup timing. # This is the commit message juanfont#20: integration/auth: improve error messages and logging for debugging - Add comprehensive timestamp logging throughout all relogin tests - Enhance EventuallyWithT error messages with detailed failure context - Add node count expectations and actual values in assertion messages - Improve validation failure reporting with state transition details - Add visual indicators (✅❌) and timeout remaining information - Include machine key and user information in debug logs for node tracking - Standardize error message format across all auth tests Addresses Phase 4.2 of relogin test flakiness plan - provides much better debugging information for timing issues and state validation failures. # This is the commit message juanfont#21: integration/helpers: remove emojis from logging output Replace visual indicators with clean text output for better compatibility across different terminal environments and cleaner log files. # This is the commit message juanfont#22: remove plan Signed-off-by: Kristoffer Dalby <[email protected]> # This is the commit message juanfont#23: integration: fix single-node validation logic in requireAllClientsOnline - Add special handling for single-node scenarios in map response validation - For single node tests, check that map responses exist rather than peer visibility - Revert OIDC test changes to maintain original security-correct logic - Original test expects both user nodes to be online in OIDC multi-user scenario # This is the commit message juanfont#24: integration/auth: implement security-focused OIDC node validation - Only validate active user's node as online (security requirement) - Maintain strict node count validation (exactly 2 nodes, no proliferation) - Preserve all existing security validations (machine key sharing, node key isolation) - Remove validateReloginComplete calls that expected both nodes online - Add explicit validation that only current user's node is online Addresses security concerns about node takeover and ensures proper active/inactive node state management in OIDC multi-user scenarios. # This is the commit message juanfont#25: db Signed-off-by: Kristoffer Dalby <[email protected]> # This is the commit message juanfont#26: Add detailed Docker build error output for integration tests When Docker builds fail in integration tests, we now get detailed error output instead of just 'returned a non-zero code: 1'. This helps with debugging build failures by showing the actual Go compilation errors or other build issues. The solution runs a manual 'docker build' command when the main dockertest build fails, capturing the full output to include in the error message. # This is the commit message juanfont#27: Improve integration test validation with staged offline checking - Split requireAllClientsOnline into separate online/offline validation paths - Add requireAllClientsOfflineStaged for component-specific timeout handling - Implement 3-stage offline validation: 1. Batcher disconnection (immediate, 15s timeout) 2. NodeStore offline status (20s timeout for disconnect detection delay) 3. Map response propagation (60s timeout for peer update delays) - Add detailed timing logs for validation stages - Improve error reporting with better context and timing information This addresses flaky integration tests by properly handling the different timing characteristics of each system component during node disconnection. # This is the commit message juanfont#28: Add comprehensive relogin test improvement plan Document the systematic approach to fixing flaky relogin integration tests: - Analysis of 5 relogin test cases and their specific issues - Staged validation approach with component-specific timeouts - EventuallyWithT pattern standardization for external calls - Infrastructure vs code issue detection strategies - Sequential execution plan for addressing each test individually This plan tracks the completed Phase 1 (validation functions) and Phase 2 (EventuallyWithT patterns) improvements, with detailed analysis of remaining test-specific issues and their root causes.
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.
cmd/headscale/headscale.gowas getting already a bit out of control. These commits changes a bit the structure for the sake of tidiness, but dumping into theclipackage the CLI funcs.