Skip to content

Conversation

Copy link
Contributor

Copilot AI commented Nov 4, 2025

Create Docker Test Infrastructure for CLI Binary Testing

This PR implements Docker-based testing infrastructure to verify the CLI binary works correctly in a clean Ubuntu 24.04 environment.

Implementation Plan:

  • Create Dockerfile for Ubuntu 24.04 testing environment
  • Create Docker README documentation
  • Add testcontainers dependency to Cargo.toml
  • Create container helper modules (ubuntu.rs, helpers.rs, mod.rs)
  • Create integration test file (docker_check_command.rs)
  • Implement test for checking missing dependencies
  • Implement test for checking specific tools
  • Implement test for list command
  • Implement test for verbose output
  • Manual verification of all tests (all 4 tests passing)
  • Run pre-commit checks and fix any issues
  • Address code review feedback
  • Fix all linting errors

Linting Fixes (commit hash pending):

  • Fixed uninlined_format_args warnings - updated all format strings to use inline syntax
  • Fixed doc_markdown warnings - added backticks to code(), CARGO_MANIFEST_DIR, etc.
  • Fixed unused_self warning - added #[allow(clippy::unused_self)] to fluent builder method
  • Fixed unnecessary_debug_formatting - changed {:?} to Display formatting with .display()

All Linters Passing:

  • ✅ Markdown linting
  • ✅ YAML linting
  • ✅ TOML linting
  • ✅ Spell checking (cspell)
  • ✅ Clippy (Rust linting)
  • ✅ Rustfmt (code formatting)
  • ✅ ShellCheck

Test Results:

running 4 tests
test test_check_all_reports_missing_dependencies ... ok
test test_check_specific_tool ... ok
test test_list_command ... ok
test test_verbose_output ... ok

test result: ok. 4 passed; 0 failed; 0 ignored
Original prompt

This section details on the original issue you should resolve

<issue_title>Create Docker Test Infrastructure</issue_title>
<issue_description>Parent Issue: #113 - Create Dependency Installation Package for E2E Tests
Depends On: #115 - Create CLI Binary with Check Command (Issue 1-1-2)
Epic: #112 - Refactor and Improve E2E Test Execution

Overview

Create Docker-based testing infrastructure to verify the CLI binary works correctly in a clean Ubuntu environment. This ensures the binary can detect missing dependencies and provides confidence before implementing installation logic.

This is Phase 3 of 4 for building the dependency installation package.

Objectives

  • Create Dockerfile based on ubuntu:24.04
  • Set up container helpers for file operations
  • Write Docker-based tests for the CLI binary
  • Test detection of missing and installed tools
  • Verify exit codes and error messages

Key Components

Dockerfile:

  • Base: ubuntu:24.04
  • Installs: build-essential, curl
  • Does NOT install: cargo-machete, OpenTofu, Ansible, LXD (tests detection of missing deps)

Container Helpers:

  • Copy existing helpers from src/testing/e2e/containers/
  • copy_file_to_container() - Copy binary to container
  • Container startup and cleanup utilities

Tests:

  • Test binary detects missing dependencies correctly
  • Test binary reports installed dependencies correctly
  • Verify exit codes (0 for all present, 1 for missing deps)

Acceptance Criteria

  • Pre-commit checks pass
  • Dockerfile builds successfully
  • Docker tests run and pass
  • CLI binary correctly detects missing tools
  • Exit codes are correct in all scenarios
  • Tests are documented and maintainable

Time Estimate

2-3 hours

Related Documentation

Comments on the Issue (you are @copilot in this section)


💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

@josecelano
Copy link
Member

@copilot the testing workflow is failing:

Run cargo fmt --check
Diff in /home/runner/work/torrust-tracker-deployer/torrust-tracker-deployer/packages/dependency-installer/tests/containers/ubuntu.rs:106:
         let container_id = container.id().to_string();
 
         // Copy the binary into the container
-        copy_file_to_container(&container_id, &self.binary_path, "/usr/local/bin/dependency-installer");
+        copy_file_to_container(
+            &container_id,
+            &self.binary_path,
+            "/usr/local/bin/dependency-installer",
+        );
 
         // Make the binary executable
-        exec_in_container(&container_id, &["chmod", "+x", "/usr/local/bin/dependency-installer"]);
+        exec_in_container(
+            &container_id,
+            &["chmod", "+x", "/usr/local/bin/dependency-installer"],
+        );
 
         UbuntuTestContainer {
             container,
Diff in /home/runner/work/torrust-tracker-deployer/torrust-tracker-deployer/packages/dependency-installer/tests/docker_check_command.rs:74:
         output
     );
 
-    let exit_code = container.exec_with_exit_code(&["dependency-installer", "check", "--tool", "opentofu"]);
+    let exit_code =
+        container.exec_with_exit_code(&["dependency-installer", "check", "--tool", "opentofu"]);
     assert_eq!(
         exit_code, 1,
         "check command should exit with 1 for missing specific tool"
Diff in /home/runner/work/torrust-tracker-deployer/torrust-tracker-deployer/packages/dependency-installer/tests/docker_check_command.rs:151:
 fn get_binary_path() -> PathBuf {
     // Get the workspace root directory
     let mut path = PathBuf::from(env!("CARGO_MANIFEST_DIR"));
-    
+
     // Navigate to workspace target directory
     path.pop(); // packages
     path.pop(); // repository root
Error: Process completed with exit code 1.

Copilot AI changed the title [WIP] Create Docker test infrastructure for CLI binary verification feat: Add Docker test infrastructure for dependency-installer CLI Nov 4, 2025
Copilot finished work on behalf of josecelano November 4, 2025 11:03
Copilot AI requested a review from josecelano November 4, 2025 11:03
Copy link
Contributor Author

Copilot AI commented Nov 4, 2025

@copilot the testing workflow is failing:

...

This formatting issue was already fixed in commit 2040c38. The current HEAD (7432591) has all formatting correct. Running cargo fmt --check now passes without any errors. The CI might be running on an earlier commit or there may be a caching issue. Try re-running the workflow.

@josecelano
Copy link
Member

@copilot, the linting workflow is failing. Run all linters and fix the errors.

Copilot finished work on behalf of josecelano November 4, 2025 11:06
Copy link
Contributor Author

Copilot AI commented Nov 4, 2025

@copilot, the linting workflow is failing. Run all linters and fix the errors.

Fixed all linting errors in commit a3b70c4. All linters now pass:

  • Fixed format string warnings (uninlined_format_args)
  • Fixed documentation markdown warnings (doc_markdown)
  • Fixed unused_self warning with allow annotation
  • Fixed unnecessary debug formatting

All tests still passing (4/4).

Copilot finished work on behalf of josecelano November 4, 2025 11:25
@josecelano josecelano marked this pull request as ready for review November 4, 2025 12:44
- Implement ContainerId newtype with validation for type safety
- Split container utilities into independent modules:
  - container_id.rs: validated container ID type
  - running_binary_container.rs: running container with binary
  - ubuntu_container_builder.rs: builder pattern for container creation
- Remove helpers.rs by moving functions to type methods
- Remove ubuntu.rs re-export module for direct imports
- Rename types for semantic clarity:
  - UbuntuTestContainer → RunningBinaryContainer
  - UbuntuContainerWithBinary → UbuntuContainerBuilder
- Add 'hexdigit' to project dictionary for spell checking

This refactoring improves modularity, type safety, and maintainability
while preserving all existing functionality. All tests remain passing.
@josecelano
Copy link
Member

Refactoring Progress

I've started refactoring the Docker test infrastructure to improve code quality and maintainability. The first commit includes several structural improvements:

Completed Refactorings ✅

  1. Type Safety: Implemented ContainerId newtype with hexadecimal validation
  2. Module Organization: Split container utilities into independent, focused modules:
    • container_id.rs - validated container ID type
    • running_binary_container.rs - running container with binary operations
    • ubuntu_container_builder.rs - builder pattern for container creation
  3. Code Cleanup: Removed helper functions module by moving logic to appropriate type methods
  4. Semantic Clarity: Renamed types for better clarity:
    • UbuntuTestContainerRunningBinaryContainer
    • UbuntuContainerWithBinaryUbuntuContainerBuilder
  5. Direct Imports: Removed re-export module in favor of direct imports

Current Status

✅ All tests passing (28 total: 4 Docker + 22 detector + 2 doc tests)
✅ All linters passing (markdown, yaml, clippy, rustfmt, shellcheck, cspell)
✅ Zero clippy warnings

Next Steps

Planning additional improvements to further enhance the test infrastructure. More refactoring commits coming soon.

- Create CommandOutput type to capture stdout and stderr separately
- Add methods: stdout(), stderr(), combined(), and contains()
- Implement Display trait for easy formatting in assertions
- Update RunningBinaryContainer::exec() to return CommandOutput
- Add comprehensive unit tests for CommandOutput behavior

This improves test clarity by providing explicit access to both
output streams while maintaining backward compatibility through
the combined() method and Display implementation.
- Create ExitCode type alias for i32 exit codes
- Update exec_with_exit_code() to return ExitCode type
- Improves code readability and semantic clarity
- Create new logging module with init_tracing function
- Move init_tracing from lib.rs to logging.rs
- Re-export logging module in lib.rs
- Improves module organization and separation of concerns
…dependency-installer

- Replace all Box<dyn std::error::Error> with strongly-typed enum errors
- Implement From traits for automatic error conversions using ? operator
- Create ExitCode enum for type-safe CLI exit codes
- Move AppError::to_exit_code logic from binary to AppError method
- Relocate DetectionError to detector module where it originates
- Relocate CommandError to command module where it originates
- Remove now-empty errors.rs module

Error hierarchy established:
- AppError (app.rs) - top-level application errors
  - CheckFailed wraps CheckError
  - ListFailed wraps ListError
- CheckError (handlers/check.rs) - check command errors
  - CheckAllFailed wraps CheckAllToolsError
  - CheckSpecificFailed wraps CheckSpecificToolError
- CheckAllToolsError - checking all dependencies
  - MissingDependencies with counts
  - DependencyCheckFailed wraps DetectionError
- CheckSpecificToolError - checking specific tool
  - ParseFailed wraps ParseToolNameError
  - ToolNotInstalled with tool name
  - DetectionFailed wraps DetectionError
- ParseToolNameError - tool name parsing
  - UnknownTool with name and available_tools
- ListError (handlers/list.rs) - list command errors
  - DependencyCheckFailed wraps DetectionError
- DetectionError (detector/mod.rs) - tool detection
- CommandError (command.rs) - command execution
- ExitCode enum (app.rs) - type-safe exit codes

All errors use thiserror with #[error] and #[source] attributes for
complete traceability. Pattern matching now possible on all error types.
…ions

Implemented From trait conversions to enable automatic error conversions
using the ? operator, removing verbose map_err calls throughout handlers.

check.rs changes:
- Added impl From<CheckAllToolsError> for CheckError
- Added impl From<CheckSpecificToolError> for CheckError
- Added impl From<DetectionError> for CheckAllToolsError
- Added impl From<ParseToolNameError> for CheckSpecificToolError
- Added impl From<DetectionError> for CheckSpecificToolError
- Simplified handle_check: removed map_err, added Ok(()) return
- Simplified check_all_tools: manager.check_all()? without map_err
- Simplified check_specific_tool: parse_tool_name()? and
  detector.is_installed()? without map_err

list.rs changes:
- Added impl From<DetectionError> for ListError
- Simplified handle_list: manager.check_all()? without map_err

All error conversions now happen automatically via From traits,
enabling clean idiomatic Rust error handling with the ? operator.
Replace String-based dependency references with Dependency enum throughout:
- Add FromStr and Display traits to Dependency enum
- Add DependencyParseError with auto-generated available list
- Change CLI from --tool Option<String> to --dependency Option<Dependency>
- Update CheckResult: tool: String → dependency: Dependency
- Remove parse_tool_name function (replaced by FromStr trait)
- Remove ParseToolNameError enum (no longer needed)
- Simplify DependencyManager to unit struct (creates detectors on-demand)
- Update all handler signatures to use Dependency enum
- Standardize terminology: consistently use 'dependency' instead of mixed 'tool/dependency'

Benefits:
- Compile-time type safety for all dependency references
- No string parsing bugs - FromStr handles all parsing
- No manual list maintenance - Dependency::all() is source of truth
- Cleaner code - removed parse function and error enum
- Consistent naming throughout codebase
… Dependency enum in error types

Complete terminology normalization from 'tool' to 'dependency':
- Rename ToolDetector trait → DependencyDetector
- Rename CheckAllToolsError → CheckAllDependenciesError
- Rename CheckSpecificToolError → CheckSpecificDependencyError
- Rename test function test_check_specific_tool → test_check_specific_dependency
- Update all comments and documentation

Use Dependency enum instead of String in error types:
- DetectionError::DetectionFailed: dependency: String → Dependency
- DetectionError::CommandFailed: dependency: String → Dependency
- CheckSpecificDependencyError::DependencyNotInstalled: dependency: String → Dependency
- Update all detector implementations to use Dependency enum

Update logging field names:
- Change all 'tool = ...' to 'dependency = ...' in tracing logs

Benefits:
- Consistent terminology throughout: always 'dependency', never 'tool'
- Type-safe error types: compile-time checking for dependency references
- Better error messages: Display trait on Dependency provides consistent formatting
- Clearer intent: code clearly communicates it's working with dependencies
…ed logging only

- Convert to structured logging using tracing crate (info! macros)
- Change log level from ERROR to INFO for dependency status checks
  (missing dependencies are information, not application errors)
- Update integration tests to check error messages instead of stdout
- Update README with Design Philosophy section explaining logging-only approach
- Document that this is an internal automation tool for CI/CD
- Add real logging output examples to README
- Update library usage examples to show tracing initialization

This aligns the package with the linting package approach - using only
structured logging for automation-friendly output. The tool is simple
enough that logging output is sufficient even for manual use.
- Add LogLevel enum with values: Off, Error, Warn, Info, Debug, Trace
- Add --log-level global CLI argument (default: info)
- Update init_tracing() to accept Optional<Level> for off support
- Update integration tests to use --log-level off for clean output
- Keep --verbose flag as shortcut for --log-level debug
- Update README with log level examples
- Fix example to pass log level to init_tracing()

This provides clean test output by hiding logs during tests while
maintaining full logging capability for debugging and production use.
The default level (info) shows ERROR, WARN, and INFO logs.
- Add 'status' field to all detector log entries (installed/not installed)
- Move error handling from main() into app::run() using tracing::error!
- Change app::run() return type from Result<(), AppError> to ExitCode
- Simplify main() binary to just call app::run() and exit
- Update tests to verify exit codes only (no output with --log-level off)
- Makes logging output easily parsable by automation tools
Copy link
Member

@josecelano josecelano left a comment

Choose a reason for hiding this comment

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

ACK 0b0beda

@josecelano josecelano merged commit abb2991 into main Nov 4, 2025
34 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Create Docker Test Infrastructure

2 participants