diff --git a/.github/skills/write-unit-test/skill.md b/.github/skills/write-unit-test/skill.md index 55899d1da..e1941c9b4 100644 --- a/.github/skills/write-unit-test/skill.md +++ b/.github/skills/write-unit-test/skill.md @@ -28,12 +28,16 @@ This skill guides you through writing unit tests that follow project conventions What are you testing? ├── Domain entity/value object? │ └── → Phase 1: Simple unit test with naming conventions +├── New test with repeated setup code? +│ └── → Phase 2: Check existing tests for duplicate patterns +├── Duplicate setup code across tests? +│ └── → Phase 3: Extract helper functions and avoid coupling ├── Time-dependent code? -│ └── → Phase 2: Use MockClock for deterministic time +│ └── → Phase 4: Use MockClock for deterministic time ├── File operations? -│ └── → Phase 3: Use TempDir for isolation +│ └── → Phase 5: Use TempDir for isolation ├── Multiple input/output combinations? -│ └── → Phase 4: Use parameterized tests (rstest) +│ └── → Phase 6: Use parameterized tests (rstest) └── Command or handler? └── → See write-integration-test skill instead ``` @@ -152,12 +156,6 @@ test domain::environment_name::tests::it_should_create_valid_name_when_using_low **Commit**: `test: add unit tests for EnvironmentName validation` -## Phase 2: Time-Dependent Tests with MockClock - -**When to use**: Testing code that uses `Utc::now()` or time-based logic. - -**Why**: Direct use of `Utc::now()` makes tests non-deterministic. - ### Step 1: Inject Clock Dependency **Production code**: @@ -249,7 +247,361 @@ mod tests { **Commit**: `test: add time-dependent tests using MockClock` -## Phase 3: Isolated Tests with TempDir +## Phase 3: Avoiding Duplicate Test Code + +**When to use**: When you notice repeated setup code across multiple tests. + +**Why**: DRY principle applies to tests - duplicate code makes tests harder to maintain and increases the risk of inconsistencies. + +### Step 1: Identify Duplicate Code Patterns + +**Watch for these code smells**: + +- ❌ **Repeated Arrange sections** - Same setup code copy-pasted across tests +- ❌ **Coupled helpers** - Helper functions that internally call other helpers with hardcoded values +- ❌ **Magic values** - Hardcoded test data scattered throughout tests +- ❌ **Complex setup** - More than 5-10 lines of boilerplate in Arrange section + +**Example of duplicate code** (BAD): + +```rust +#[test] +fn it_should_convert_environment_to_dto() { + // Arrange - DUPLICATED SETUP + let env_name = EnvironmentName::new("test-env".to_string()).unwrap(); + let ssh_username = Username::new("deployer".to_string()).unwrap(); + let ssh_credentials = SshCredentials::new( + PathBuf::from("./keys/test_rsa"), + PathBuf::from("./keys/test_rsa.pub"), + ssh_username, + ); + let provider_config = ProviderConfig::Lxd(LxdConfig { + profile_name: ProfileName::new("lxd-test".to_string()).unwrap(), + }); + let created_at = Utc.with_ymd_and_hms(2026, 2, 23, 10, 0, 0).unwrap(); + let env = Environment::new(env_name, provider_config, ssh_credentials, 22, created_at); + // ... test logic +} + +#[test] +fn it_should_handle_missing_ip() { + // Arrange - SAME DUPLICATED SETUP AGAIN + let env_name = EnvironmentName::new("test-env".to_string()).unwrap(); + let ssh_username = Username::new("deployer".to_string()).unwrap(); + let ssh_credentials = SshCredentials::new( + PathBuf::from("./keys/test_rsa"), + PathBuf::from("./keys/test_rsa.pub"), + ssh_username, + ); + // ... copies continue +} +``` + +### Step 2: Extract Helper Functions + +**Rule of thumb**: If you copy code 2+ times, extract it. + +**Pattern**: Create focused helper functions for different aspects of setup: + +```rust +#[cfg(test)] +mod tests { + use super::*; + + // Test fixtures and helpers section + + fn create_test_ssh_credentials() -> SshCredentials { + let ssh_username = Username::new("deployer".to_string()).unwrap(); + SshCredentials::new( + PathBuf::from("./keys/test_rsa"), + PathBuf::from("./keys/test_rsa.pub"), + ssh_username, + ) + } + + fn create_test_provider_config() -> ProviderConfig { + ProviderConfig::Lxd(LxdConfig { + profile_name: ProfileName::new("lxd-test-env".to_string()).unwrap(), + }) + } + + fn create_test_timestamp() -> DateTime { + Utc.with_ymd_and_hms(2026, 2, 23, 10, 0, 0).unwrap() + } + + fn create_test_ip() -> IpAddr { + IpAddr::V4(Ipv4Addr::new(10, 140, 190, 39)) + } + + // Higher-level builder that composes the parts + fn create_configured_environment_with_ip(ip: IpAddr) -> Environment { + let env_name = EnvironmentName::new("test-env".to_string()).unwrap(); + let ssh_credentials = create_test_ssh_credentials(); + let provider_config = create_test_provider_config(); + let created_at = create_test_timestamp(); + + Environment::new(env_name, provider_config, ssh_credentials, 22, created_at) + .start_provisioning() + .provisioned(ip, ProvisionMethod::Provisioned) + .start_configuring() + .configured() + } + + // Tests section + + #[test] + fn it_should_convert_configured_environment_to_dto() { + // Arrange - Now concise and clear! + let test_ip = create_test_ip(); + let env = create_configured_environment_with_ip(test_ip); + + // Act + let dto = ConfigureDetailsData::from(&env); + + // Assert + assert_eq!(dto.instance_ip, Some(test_ip)); + } +} +``` + +### Step 3: Avoid Coupling Between Helpers + +**Problem**: Helpers that call each other with hardcoded values create hidden dependencies. + +**Anti-pattern - Coupled helpers** (BAD): + +```rust +// ❌ BAD: create_expected_dto() internally calls create_test_ip() +fn create_expected_dto() -> ConfigureDetailsData { + ConfigureDetailsData { + instance_ip: Some(create_test_ip()), // Hardcoded dependency + // ... + } +} + +#[test] +fn test_conversion() { + let env = create_configured_environment_with_ip(create_test_ip()); + let expected = create_expected_dto(); // Uses different IP internally! + assert_eq!(ConfigureDetailsData::from(&env), expected); +} +``` + +**Fixed - Decoupled helpers** (GOOD): + +```rust +// ✅ GOOD: Accept parameter to stay flexible and explicit +fn create_expected_dto(ip: IpAddr) -> ConfigureDetailsData { + ConfigureDetailsData { + instance_ip: Some(ip), // Use provided IP + // ... + } +} + +#[test] +fn test_conversion() { + // Arrange: Single source of truth + let test_ip = create_test_ip(); + let env = create_configured_environment_with_ip(test_ip); + let expected = create_expected_dto(test_ip); // Same IP, explicit + + // Act & Assert + assert_eq!(ConfigureDetailsData::from(&env), expected); +} +``` + +**Benefits**: + +- ✅ **No hidden dependencies** - All inputs are explicit +- ✅ **Single source of truth** - Test data defined once +- ✅ **Easy to vary** - Can test different IPs without changing helpers +- ✅ **Clear intent** - Obvious that both use the same value + +### Step 4: Use Derives to Simplify Assertions + +**Add `PartialEq` to DTOs** to enable single-line assertions: + +```rust +// In production code +#[derive(Debug, Clone, PartialEq, Serialize)] // Add PartialEq +pub struct ConfigureDetailsData { + pub environment_name: String, + pub instance_name: String, + // ... +} +``` + +**Before** - Field-by-field assertions: + +```rust +// ❌ Verbose: 6 separate assertions +assert_eq!(dto.environment_name, "test-env"); +assert_eq!(dto.instance_name, "torrust-tracker-vm-test-env"); +assert_eq!(dto.provider, "lxd"); +assert_eq!(dto.state, "Configured"); +assert_eq!(dto.instance_ip, Some(test_ip)); +assert_eq!(dto.created_at, expected_created_at); +``` + +**After** - Single assertion: + +```rust +// ✅ Concise: One assertion, better error messages +let expected = create_expected_dto(test_ip); +assert_eq!(dto, expected); +``` + +### Step 5: Organize Test Modules + +**Pattern**: Separate helpers from tests with clear sections: + +```rust +#[cfg(test)] +mod tests { + use super::*; + // Imports... + + // ======================================== + // Test fixtures and helpers + // ======================================== + + fn create_test_ssh_credentials() -> SshCredentials { /* ... */ } + fn create_test_provider_config() -> ProviderConfig { /* ... */ } + fn create_configured_environment_with_ip(ip: IpAddr) -> Environment { /* ... */ } + + // ======================================== + // Tests + // ======================================== + + #[test] + fn it_should_convert_configured_environment_to_dto() { /* ... */ } + + #[test] + fn it_should_handle_none_instance_ip() { /* ... */ } +} +``` + +**Benefits**: + +- ✅ **Clear separation** - Helpers vs actual tests +- ✅ **Easy navigation** - Find what you need quickly +- ✅ **Reusability** - Helper functions available to all tests in module + +### Step 6: Create Meaningful Assertion Helpers + +**Problem**: Repetitive assertion patterns make tests verbose and harder to maintain. + +**Pattern**: Extract repeated assertion logic into descriptive helper functions. + +**Anti-pattern - Multiple similar assertions** (BAD): + +```rust +#[test] +fn it_should_render_text_output() { + let text = render_output(&data); + + // ❌ BAD: 13 repetitive assertions + assert!(text.contains("Environment Details:")); + assert!(text.contains("Name:")); + assert!(text.contains("test-env")); + assert!(text.contains("Instance:")); + assert!(text.contains("torrust-tracker-vm-test-env")); + assert!(text.contains("Provider:")); + assert!(text.contains("lxd")); + assert!(text.contains("State:")); + assert!(text.contains("Configured")); + assert!(text.contains("Instance IP:")); + assert!(text.contains("10.140.190.39")); + assert!(text.contains("Created:")); + assert!(text.contains("2026-02-23 10:00:00 UTC")); +} +``` + +**Fixed - Single assertion helper** (GOOD): + +```rust +// Test fixtures and helpers + +/// Helper to assert text contains all expected substrings +fn assert_contains_all(text: &str, expected: &[&str]) { + for substring in expected { + assert!( + text.contains(substring), + "Expected text to contain '{}' but it didn't.\nActual text:\n{}", + substring, + text + ); + } +} + +// Tests + +#[test] +fn it_should_render_text_output() { + let text = render_output(&data); + + // ✅ GOOD: Single assertion with clear expectations + assert_contains_all( + &text, + &[ + "Environment Details:", + "Name:", + "test-env", + "Instance:", + "torrust-tracker-vm-test-env", + "Provider:", + "lxd", + "State:", + "Configured", + "Instance IP:", + "10.140.190.39", + "Created:", + "2026-02-23 10:00:00 UTC", + ], + ); +} +``` + +**When to create assertion helpers**: + +- ✅ **3+ similar assertions** - Pattern emerges that can be abstracted +- ✅ **Repeated validation logic** - Same check used across multiple tests +- ✅ **Complex validation** - Multi-step verification that obscures test intent +- ✅ **Error clarity matters** - Custom messages improve debugging experience + +**Common assertion helper patterns**: + +```rust +// Pattern 1: Contains all strings +fn assert_contains_all(text: &str, expected: &[&str]) { /* ... */ } + +// Pattern 2: JSON field validation +fn assert_json_fields(json: &str, fields: &[(&str, &str)]) { /* ... */ } + +// Pattern 3: Collection validation +fn assert_collection_contains(collection: &[T], predicate: impl Fn(&T) -> bool) { /* ... */ } + +// Pattern 4: State validation +fn assert_valid_state(obj: &MyType, expectations: &StateExpectations) { /* ... */ } +``` + +**Benefits**: + +- ✅ **Reduced verbosity** - 10+ assertions → 1 function call +- ✅ **Better readability** - Clear list of expectations +- ✅ **Maintainability** - Change validation logic in one place +- ✅ **Clear error messages** - Custom messages show what failed and why +- ✅ **Reusability** - Use same helper across multiple tests + +**See**: PR [#373](https://github.com/torrust/torrust-tracker-deployer/pull/373) for complete example of refactoring duplicate test code. + +**Commit**: `refactor: extract duplicate test code and decouple test setup` + +## Phase 4: Time-Dependent Tests with MockClock + +**When to use**: Testing code that uses `Utc::now()` or time-based logic. + +**Why**: Direct use of `Utc::now()` makes tests non-deterministic. **When to use**: Testing code that creates files, directories, or modifies filesystem. @@ -323,7 +675,7 @@ fn bad_test_creates_real_directories() { **Commit**: `test: add filesystem tests using TempDir` -## Phase 4: Parameterized Tests with rstest +## Phase 6: Parameterized Tests with rstest **When to use**: Testing same behavior with multiple input/output combinations. @@ -435,7 +787,7 @@ fn it_should_create_correct_paths_for_different_environments( **Commit**: `test: add parameterized tests for input validation` -## Phase 5: Verify and Fix +## Phase 7: Verify and Fix ### Step 1: Run Tests diff --git a/AGENTS.md b/AGENTS.md index 0380e9427..62d80574d 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -65,6 +65,19 @@ Both production and test code must be: - **Readable**: Clear intent that can be understood by other developers - **Testable**: Designed to support comprehensive testing at all levels +**Beck's Four Rules of Simple Design:** + +Follow Kent Beck's four rules of simple design (in priority order): + +1. **Passes the tests**: The code must work as intended - testing is a first-class activity +2. **Reveals intention**: Code should be easy to understand, expressing purpose clearly +3. **No duplication**: Apply DRY (Don't Repeat Yourself) / Once and Only Once - eliminating duplication drives out good designs +4. **Fewest elements**: Remove anything that doesn't serve the prior three rules - avoid premature optimization for hypothetical future requirements + +These rules feed off each other in refining code and apply to any language or paradigm. When in conflict, empathy for the reader wins over strict technical metrics. + +Reference: [Beck Design Rules](https://martinfowler.com/bliki/BeckDesignRules.html) + These principles should guide all development decisions, code reviews, and feature implementations. ## 🔧 Essential Rules @@ -216,7 +229,7 @@ The project provides Agent Skills in `.github/skills/` for specialized workflows Available skills: | Task | Skill to Load | -| ---------------------------- | --------------------------------------------------- | --- | --------------------- | --------------------------------------------- | --- | --------------------------- | -------------------------------------------------- | +| ---------------------------- | --------------------------------------------------- | | Adding commands | `.github/skills/add-new-command/skill.md` | | Cleaning up completed issues | `.github/skills/cleanup-completed-issues/skill.md` | | Cleaning LXD environments | `.github/skills/clean-lxd-environments/skill.md` | @@ -229,7 +242,9 @@ Available skills: | Creating feature specs | `.github/skills/create-feature-spec/skill.md` | | Creating issues | `.github/skills/create-issue/skill.md` | | Creating new skills | `.github/skills/add-new-skill/skill.md` | -| Creating refactor plans | `.github/skills/create-refactor-plan/skill.md` | | Regenerating CLI docs | `.github/skills/regenerate-cli-docs/skill.md` | | Rendering tracker artifacts | `.github/skills/render-tracker-artifacts/skill.md` | +| Creating refactor plans | `.github/skills/create-refactor-plan/skill.md` | +| Regenerating CLI docs | `.github/skills/regenerate-cli-docs/skill.md` | +| Rendering tracker artifacts | `.github/skills/render-tracker-artifacts/skill.md` | | Reviewing pull requests | `.github/skills/review-pr/skill.md` | | Running linters | `.github/skills/run-linters/skill.md` | | Running local E2E tests | `.github/skills/run-local-e2e-test/skill.md` | diff --git a/docs/contributing/pr-review-guide.md b/docs/contributing/pr-review-guide.md index ae5b85133..ba5239e53 100644 --- a/docs/contributing/pr-review-guide.md +++ b/docs/contributing/pr-review-guide.md @@ -32,6 +32,8 @@ Verify contributors followed these project conventions: - [ ] **Tests follow naming conventions** (`it_should_...` pattern) - [ ] **Tests are isolated** - use temporary resources, don't depend on external state - [ ] **Tests are readable** - clear intent and easy to understand what's being tested +- [ ] **Test setup code is DRY** - no duplicate Arrange sections across tests +- [ ] **Test fixtures are decoupled** - helper functions use parameters, not hardcoded internal calls - [ ] **Both production and test code** meet quality standards (clean, maintainable, sustainable) ### Documentation @@ -80,6 +82,9 @@ Watch for these common issues that indicate quality problems: - ❌ **Missing tests for new error paths** - Error handling should be tested - ❌ **Tests that depend on external state** - Tests should be isolated - ❌ **Test code that doesn't meet production quality standards** - Both should be clean +- ❌ **Duplicate test setup code** - Extract to helper functions (see PR [#373](https://github.com/torrust/torrust-tracker-deployer/pull/373)) +- ❌ **Coupled helper functions** - Parameterize helpers instead of hardcoding dependencies +- ❌ **Field-by-field assertions on DTOs** - Add `PartialEq` and assert equality directly ### Known Issues vs. Real Problems diff --git a/src/presentation/controllers/configure/handler.rs b/src/presentation/controllers/configure/handler.rs index 134d38e82..bcdf22904 100644 --- a/src/presentation/controllers/configure/handler.rs +++ b/src/presentation/controllers/configure/handler.rs @@ -13,6 +13,8 @@ use crate::domain::environment::name::EnvironmentName; use crate::domain::environment::repository::EnvironmentRepository; use crate::domain::environment::state::Configured; use crate::domain::environment::Environment; +use crate::presentation::input::cli::OutputFormat; +use crate::presentation::views::commands::configure::{ConfigureDetailsData, JsonView, TextView}; use crate::presentation::views::progress::ProgressReporter; use crate::presentation::views::progress::VerboseProgressListener; use crate::presentation::views::UserOutput; @@ -101,11 +103,13 @@ impl ConfigureCommandController { /// 2. Load and validate environment state /// 3. Create command handler /// 4. Configure infrastructure - /// 5. Complete with success message + /// 5. Display results (in specified format) + /// 6. Complete with success message /// /// # Arguments /// /// * `environment_name` - The name of the environment to configure + /// * `output_format` - The output format (Text or Json) /// /// # Errors /// @@ -123,6 +127,7 @@ impl ConfigureCommandController { pub fn execute( &mut self, environment_name: &str, + output_format: OutputFormat, ) -> Result, ConfigureSubcommandError> { let env_name = self.validate_environment_name(environment_name)?; @@ -132,6 +137,8 @@ impl ConfigureCommandController { self.complete_workflow(environment_name)?; + self.display_configure_results(&configured, output_format)?; + Ok(configured) } @@ -224,6 +231,42 @@ impl ConfigureCommandController { .complete(&format!("Environment '{name}' configured successfully"))?; Ok(()) } + + /// Display configure results in the specified format + /// + /// Uses the Strategy Pattern to render configure details in either + /// human-readable text or machine-readable JSON format. + /// + /// # Arguments + /// + /// * `configured` - The configured environment to display + /// * `output_format` - The output format (Text or Json) + /// + /// # Errors + /// + /// Returns an error if: + /// - Progress reporting encounters a poisoned mutex + /// + /// # Note + /// + /// JSON serialization errors are handled inline by `JsonView::render()`, + /// which returns a fallback error JSON string. Therefore, this method + /// does not propagate serialization errors. + #[allow(clippy::result_large_err)] + fn display_configure_results( + &mut self, + configured: &Environment, + output_format: OutputFormat, + ) -> Result<(), ConfigureSubcommandError> { + self.progress.blank_line()?; + let details = ConfigureDetailsData::from(configured); + let output = match output_format { + OutputFormat::Text => TextView::render(&details), + OutputFormat::Json => JsonView::render(&details), + }; + self.progress.result(&output)?; + Ok(()) + } } #[cfg(test)] @@ -269,7 +312,7 @@ mod tests { // Test with invalid environment name (contains underscore) let result = ConfigureCommandController::new(repository, clock, user_output.clone()) - .execute("invalid_name"); + .execute("invalid_name", OutputFormat::Text); assert!(result.is_err()); match result.unwrap_err() { @@ -288,8 +331,8 @@ mod tests { let (user_output, repository, clock) = create_test_dependencies(&temp_dir); - let result = - ConfigureCommandController::new(repository, clock, user_output.clone()).execute(""); + let result = ConfigureCommandController::new(repository, clock, user_output.clone()) + .execute("", OutputFormat::Text); assert!(result.is_err()); match result.unwrap_err() { @@ -310,7 +353,7 @@ mod tests { // Try to configure an environment that doesn't exist let result = ConfigureCommandController::new(repository, clock, user_output.clone()) - .execute("nonexistent-env"); + .execute("nonexistent-env", OutputFormat::Text); assert!(result.is_err()); // After refactoring, repository NotFound error is wrapped in ConfigureOperationFailed @@ -339,7 +382,7 @@ mod tests { // Valid environment name should pass validation, but will fail // at configure operation since we don't have a real environment setup let result = ConfigureCommandController::new(repository, clock, user_output.clone()) - .execute("test-env"); + .execute("test-env", OutputFormat::Text); // Should fail at operation, not at name validation if let Err(ConfigureSubcommandError::InvalidEnvironmentName { .. }) = result { diff --git a/src/presentation/controllers/configure/tests/mod.rs b/src/presentation/controllers/configure/tests/mod.rs index c434952de..ce43a6a5f 100644 --- a/src/presentation/controllers/configure/tests/mod.rs +++ b/src/presentation/controllers/configure/tests/mod.rs @@ -15,6 +15,7 @@ mod integration_tests { use crate::presentation::controllers::configure; use crate::presentation::controllers::configure::handler::ConfigureCommandController; use crate::presentation::controllers::constants::DEFAULT_LOCK_TIMEOUT; + use crate::presentation::input::cli::OutputFormat; use crate::presentation::views::testing::TestUserOutput; use crate::presentation::views::{UserOutput, VerbosityLevel}; use crate::shared::clock::Clock; @@ -47,7 +48,7 @@ mod integration_tests { let (user_output, repository, clock) = create_test_dependencies(&temp_dir); let result = ConfigureCommandController::new(repository, clock, user_output.clone()) - .execute("invalid_name_with_underscore"); + .execute("invalid_name_with_underscore", OutputFormat::Text); assert!(result.is_err()); let error = result.unwrap_err(); @@ -65,7 +66,7 @@ mod integration_tests { let (user_output, repository, clock) = create_test_dependencies(&temp_dir); let result = ConfigureCommandController::new(repository, clock, user_output.clone()) - .execute("bad_name"); + .execute("bad_name", OutputFormat::Text); assert!(result.is_err()); let error = result.unwrap_err(); @@ -82,7 +83,7 @@ mod integration_tests { let (user_output, repository, clock) = create_test_dependencies(&temp_dir); let result = ConfigureCommandController::new(repository, clock, user_output.clone()) - .execute("nonexistent-environment"); + .execute("nonexistent-environment", OutputFormat::Text); assert!(result.is_err()); // Repository will return NotFound error, wrapped in ConfigureOperationFailed diff --git a/src/presentation/dispatch/router.rs b/src/presentation/dispatch/router.rs index e81acff8d..d1a315238 100644 --- a/src/presentation/dispatch/router.rs +++ b/src/presentation/dispatch/router.rs @@ -140,10 +140,11 @@ pub async fn route_command( Ok(()) } Commands::Configure { environment } => { + let output_format = context.output_format(); context .container() .create_configure_controller() - .execute(&environment)?; + .execute(&environment, output_format)?; Ok(()) } Commands::Test { environment } => { diff --git a/src/presentation/views/commands/configure/mod.rs b/src/presentation/views/commands/configure/mod.rs new file mode 100644 index 000000000..7dac38792 --- /dev/null +++ b/src/presentation/views/commands/configure/mod.rs @@ -0,0 +1,52 @@ +//! Views for Configure Command +//! +//! This module contains view components for rendering configure command output. +//! +//! # Architecture +//! +//! This module follows the Strategy Pattern for rendering: +//! - `ConfigureDetailsData`: The data DTO passed to all views +//! - `TextView`: Renders human-readable text output +//! - `JsonView`: Renders machine-readable JSON output +//! +//! # Structure +//! +//! - `view_data/`: Data structures (DTOs) passed to views +//! - `configure_details.rs`: Main DTO with environment configure data +//! - `views/`: View rendering implementations +//! - `text_view.rs`: Human-readable text rendering +//! - `json_view.rs`: Machine-readable JSON rendering +//! +//! # SOLID Principles +//! +//! - **Single Responsibility**: Each view has one job - render in its format +//! - **Open/Closed**: Add new formats by creating new view files, not modifying existing ones +//! - **Strategy Pattern**: Different rendering strategies for the same data +//! +//! # Adding New Formats +//! +//! To add a new output format (e.g., XML, YAML, CSV): +//! 1. Create a new file in `views/`: `xml_view.rs`, `yaml_view.rs`, etc. +//! 2. Implement the view with `render(data: &ConfigureDetailsData) -> String` +//! 3. Export it from this module +//! 4. No need to modify existing views or the DTO + +pub mod view_data { + pub mod configure_details; + + // Re-export main types for convenience + pub use configure_details::ConfigureDetailsData; +} + +pub mod views { + pub mod json_view; + pub mod text_view; + + // Re-export views for convenience + pub use json_view::JsonView; + pub use text_view::TextView; +} + +// Re-export at module root for convenience +pub use view_data::ConfigureDetailsData; +pub use views::{JsonView, TextView}; diff --git a/src/presentation/views/commands/configure/view_data/configure_details.rs b/src/presentation/views/commands/configure/view_data/configure_details.rs new file mode 100644 index 000000000..5fcaf3363 --- /dev/null +++ b/src/presentation/views/commands/configure/view_data/configure_details.rs @@ -0,0 +1,170 @@ +//! Configure Details Data Transfer Object +//! +//! This module contains the presentation DTO for configure command details. +//! It serves as the data structure passed to view renderers (`TextView`, `JsonView`, etc.). +//! +//! # Architecture +//! +//! This follows the Strategy Pattern where: +//! - This DTO is the data passed to all rendering strategies +//! - Different views (`TextView`, `JsonView`) consume this data +//! - Adding new formats doesn't modify this DTO or existing views +//! +//! # SOLID Principles +//! +//! - **Single Responsibility**: This file only defines the data structure +//! - **Open/Closed**: New formats extend by adding views, not modifying this +//! - **Separation of Concerns**: Data definition separate from rendering logic + +use chrono::{DateTime, Utc}; +use serde::Serialize; +use std::net::IpAddr; + +use crate::domain::environment::state::Configured; +use crate::domain::environment::Environment; + +/// Configure details data for rendering +/// +/// This struct holds all the data needed to render configure command +/// information for display to the user. It is consumed by view renderers +/// (`TextView`, `JsonView`) which format it according to their specific output format. +/// +/// # Design +/// +/// This is a presentation layer DTO (Data Transfer Object) that: +/// - Decouples domain models from view formatting +/// - Provides a stable interface for multiple view strategies +/// - Contains all fields needed for any output format +#[derive(Debug, Clone, PartialEq, Serialize)] +pub struct ConfigureDetailsData { + /// Name of the configured environment + pub environment_name: String, + /// Name of the configured instance + pub instance_name: String, + /// Infrastructure provider (lowercase: "lxd", "hetzner", etc.) + pub provider: String, + /// State name (always "Configured" for this command) + pub state: String, + /// IP address of the instance (nullable) + pub instance_ip: Option, + /// Timestamp when the environment was created (ISO 8601 format in JSON) + pub created_at: DateTime, +} + +/// Conversion from domain model to presentation DTO +/// +/// This `From` trait implementation is placed in the presentation layer +/// (not in the domain layer) to maintain proper DDD layering: +/// +/// - Domain layer should not depend on presentation layer DTOs +/// - Presentation layer can depend on domain models (allowed) +/// - This keeps the domain clean and focused on business logic +/// +/// Alternative approaches considered: +/// - Adding method to `Environment`: Would violate DDD by making +/// domain depend on presentation DTOs +/// - Keeping mapping in controller: Works but less idiomatic than `From` trait +impl From<&Environment> for ConfigureDetailsData { + fn from(env: &Environment) -> Self { + Self { + environment_name: env.name().as_str().to_string(), + instance_name: env.instance_name().as_str().to_string(), + provider: env.provider_config().provider_name().to_string(), + state: "Configured".to_string(), + instance_ip: env.instance_ip(), + created_at: env.created_at(), + } + } +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::adapters::ssh::SshCredentials; + use crate::domain::environment::runtime_outputs::ProvisionMethod; + use crate::domain::environment::EnvironmentName; + use crate::domain::provider::{LxdConfig, ProviderConfig}; + use crate::domain::ProfileName; + use crate::shared::Username; + use chrono::{DateTime, TimeZone, Utc}; + use std::net::{IpAddr, Ipv4Addr}; + use std::path::PathBuf; + + // Test fixtures and helpers + + fn create_test_ssh_credentials() -> SshCredentials { + let ssh_username = Username::new("deployer".to_string()).unwrap(); + SshCredentials::new( + PathBuf::from("./keys/test_rsa"), + PathBuf::from("./keys/test_rsa.pub"), + ssh_username, + ) + } + + fn create_test_provider_config() -> ProviderConfig { + ProviderConfig::Lxd(LxdConfig { + profile_name: ProfileName::new("lxd-test-env".to_string()).unwrap(), + }) + } + + fn create_test_timestamp() -> DateTime { + Utc.with_ymd_and_hms(2026, 2, 23, 10, 0, 0).unwrap() + } + + fn create_configured_environment_with_ip(ip: IpAddr) -> Environment { + let env_name = EnvironmentName::new("test-env".to_string()).unwrap(); + let ssh_credentials = create_test_ssh_credentials(); + let provider_config = create_test_provider_config(); + let created_at = create_test_timestamp(); + + Environment::new(env_name, provider_config, ssh_credentials, 22, created_at) + .start_provisioning() + .provisioned(ip, ProvisionMethod::Provisioned) + .start_configuring() + .configured() + } + + fn create_test_ip() -> IpAddr { + IpAddr::V4(Ipv4Addr::new(10, 140, 190, 39)) + } + + fn create_expected_dto(ip: IpAddr) -> ConfigureDetailsData { + ConfigureDetailsData { + environment_name: "test-env".to_string(), + instance_name: "torrust-tracker-vm-test-env".to_string(), + provider: "lxd".to_string(), + state: "Configured".to_string(), + instance_ip: Some(ip), + created_at: create_test_timestamp(), + } + } + + // Tests + + #[test] + fn it_should_convert_configured_environment_to_dto() { + // Arrange + let test_ip = create_test_ip(); + let env = create_configured_environment_with_ip(test_ip); + let expected = create_expected_dto(test_ip); + + // Act + let dto = ConfigureDetailsData::from(&env); + + // Assert + assert_eq!(dto, expected); + } + + #[test] + fn it_should_handle_none_instance_ip() { + // Arrange - create environment with IP + let env = create_configured_environment_with_ip(create_test_ip()); + + // Act + let dto = ConfigureDetailsData::from(&env); + + // Assert - in a real configured environment, IP should always be present + // This test documents that configured environments have IP addresses + assert!(dto.instance_ip.is_some()); + } +} diff --git a/src/presentation/views/commands/configure/views/json_view.rs b/src/presentation/views/commands/configure/views/json_view.rs new file mode 100644 index 000000000..735ac4821 --- /dev/null +++ b/src/presentation/views/commands/configure/views/json_view.rs @@ -0,0 +1,213 @@ +//! JSON View for Configure Command +//! +//! This module provides JSON-based rendering for the configure command. +//! It follows the Strategy Pattern, providing a machine-readable output format +//! for the same underlying data (`ConfigureDetailsData` DTO). +//! +//! # Design +//! +//! The `JsonView` serializes configure command information to JSON using `serde_json`. +//! The output includes environment details and configuration state. + +use crate::presentation::views::commands::configure::ConfigureDetailsData; + +/// View for rendering configure details as JSON +/// +/// This view provides machine-readable JSON output for automation workflows +/// and AI agents. It serializes the configure details without any transformations, +/// preserving all field names and structure from the DTO. +/// +/// # Examples +/// +/// ```rust +/// use torrust_tracker_deployer_lib::presentation::views::commands::configure::{ +/// ConfigureDetailsData, JsonView, +/// }; +/// use chrono::{TimeZone, Utc}; +/// use std::net::{IpAddr, Ipv4Addr}; +/// +/// let details = ConfigureDetailsData { +/// environment_name: "my-env".to_string(), +/// instance_name: "torrust-tracker-vm-my-env".to_string(), +/// provider: "lxd".to_string(), +/// state: "Configured".to_string(), +/// instance_ip: Some(IpAddr::V4(Ipv4Addr::new(10, 140, 190, 39))), +/// created_at: Utc.with_ymd_and_hms(2026, 2, 20, 10, 0, 0).unwrap(), +/// }; +/// +/// let output = JsonView::render(&details); +/// +/// // Verify it's valid JSON +/// let parsed: serde_json::Value = serde_json::from_str(&output).unwrap(); +/// assert_eq!(parsed["environment_name"], "my-env"); +/// assert_eq!(parsed["state"], "Configured"); +/// ``` +pub struct JsonView; + +impl JsonView { + /// Render configure details as JSON + /// + /// Serializes the configure details to pretty-printed JSON format. + /// The JSON structure matches the DTO structure exactly: + /// - `environment_name`: Name of the environment + /// - `instance_name`: VM instance name + /// - `provider`: Infrastructure provider + /// - `state`: Always "Configured" on success + /// - `instance_ip`: IP address (nullable) + /// - `created_at`: ISO 8601 UTC timestamp + /// + /// # Arguments + /// + /// * `data` - Configure details to render + /// + /// # Returns + /// + /// A JSON string containing the serialized configure details. + /// If serialization fails (which should never happen with valid data), + /// returns an error JSON object with the serialization error message. + /// + /// # Examples + /// + /// ```rust + /// use torrust_tracker_deployer_lib::presentation::views::commands::configure::{ + /// ConfigureDetailsData, JsonView, + /// }; + /// use chrono::{TimeZone, Utc}; + /// use std::net::{IpAddr, Ipv4Addr}; + /// + /// let details = ConfigureDetailsData { + /// environment_name: "prod-tracker".to_string(), + /// instance_name: "torrust-tracker-vm-prod-tracker".to_string(), + /// provider: "lxd".to_string(), + /// state: "Configured".to_string(), + /// instance_ip: Some(IpAddr::V4(Ipv4Addr::new(192, 168, 1, 100))), + /// created_at: Utc.with_ymd_and_hms(2026, 1, 5, 10, 30, 0).unwrap(), + /// }; + /// + /// let json = JsonView::render(&details); + /// + /// assert!(json.contains("\"environment_name\": \"prod-tracker\"")); + /// assert!(json.contains("\"state\": \"Configured\"")); + /// ``` + #[must_use] + pub fn render(data: &ConfigureDetailsData) -> String { + serde_json::to_string_pretty(data).unwrap_or_else(|e| { + format!( + r#"{{ + "error": "Failed to serialize configure details", + "message": "{e}" +}}"# + ) + }) + } +} + +#[cfg(test)] +mod tests { + use super::*; + use chrono::{DateTime, TimeZone, Utc}; + use std::net::{IpAddr, Ipv4Addr}; + + // Test fixtures and helpers + + fn create_test_timestamp() -> DateTime { + Utc.with_ymd_and_hms(2026, 2, 23, 10, 0, 0).unwrap() + } + + fn create_test_ip() -> IpAddr { + IpAddr::V4(Ipv4Addr::new(10, 140, 190, 39)) + } + + fn create_test_details_with_ip(ip: Option) -> ConfigureDetailsData { + ConfigureDetailsData { + environment_name: "test-env".to_string(), + instance_name: "torrust-tracker-vm-test-env".to_string(), + provider: "lxd".to_string(), + state: "Configured".to_string(), + instance_ip: ip, + created_at: create_test_timestamp(), + } + } + + /// Helper to assert JSON fields match expected values + fn assert_json_fields_eq(json: &str, expected_fields: &[(&str, &str)]) { + let parsed: serde_json::Value = serde_json::from_str(json).expect("Should be valid JSON"); + for (field, expected_value) in expected_fields { + assert_eq!( + parsed[field].as_str().unwrap_or(""), + *expected_value, + "Field '{field}' should be '{expected_value}'" + ); + } + } + + /// Helper to assert JSON contains all required field names + fn assert_json_has_fields(json: &str, field_names: &[&str]) { + let parsed: serde_json::Value = serde_json::from_str(json).expect("Should be valid JSON"); + for field_name in field_names { + assert!( + parsed.get(field_name).is_some(), + "Expected JSON to have field '{field_name}' but it didn't.\nActual JSON:\n{json}" + ); + } + } + + // Tests + + #[test] + fn it_should_render_configure_details_as_valid_json() { + // Arrange + let details = create_test_details_with_ip(Some(create_test_ip())); + + // Act + let json = JsonView::render(&details); + + // Assert - verify it's valid JSON with expected field values + assert_json_fields_eq( + &json, + &[ + ("environment_name", "test-env"), + ("instance_name", "torrust-tracker-vm-test-env"), + ("provider", "lxd"), + ("state", "Configured"), + ("instance_ip", "10.140.190.39"), + ("created_at", "2026-02-23T10:00:00Z"), + ], + ); + } + + #[test] + fn it_should_render_null_instance_ip_as_json_null() { + // Arrange + let details = create_test_details_with_ip(None); + + // Act + let json = JsonView::render(&details); + + // Assert + let parsed: serde_json::Value = serde_json::from_str(&json).expect("Should be valid JSON"); + assert!(parsed["instance_ip"].is_null()); + } + + #[test] + fn it_should_include_all_required_fields() { + // Arrange + let details = create_test_details_with_ip(Some(IpAddr::V4(Ipv4Addr::new(192, 168, 1, 42)))); + + // Act + let json = JsonView::render(&details); + + // Assert - check all required fields are present + assert_json_has_fields( + &json, + &[ + "environment_name", + "instance_name", + "provider", + "state", + "instance_ip", + "created_at", + ], + ); + } +} diff --git a/src/presentation/views/commands/configure/views/text_view.rs b/src/presentation/views/commands/configure/views/text_view.rs new file mode 100644 index 000000000..f48531032 --- /dev/null +++ b/src/presentation/views/commands/configure/views/text_view.rs @@ -0,0 +1,223 @@ +//! Text View for Configure Command +//! +//! This module provides text-based rendering for the configure command. +//! It follows the Strategy Pattern, providing a human-readable output format +//! for the same underlying data (`ConfigureDetailsData` DTO). +//! +//! # Design +//! +//! The `TextView` formats configure details as human-readable text suitable +//! for terminal display and direct user consumption. + +use crate::presentation::views::commands::configure::ConfigureDetailsData; + +/// View for rendering configure details as human-readable text +/// +/// This view produces formatted text output suitable for terminal display +/// and human consumption. It presents environment configuration details +/// in a clear, readable format. +/// +/// # Examples +/// +/// ```rust +/// use torrust_tracker_deployer_lib::presentation::views::commands::configure::{ +/// ConfigureDetailsData, TextView, +/// }; +/// use chrono::{TimeZone, Utc}; +/// use std::net::{IpAddr, Ipv4Addr}; +/// +/// let details = ConfigureDetailsData { +/// environment_name: "my-env".to_string(), +/// instance_name: "torrust-tracker-vm-my-env".to_string(), +/// provider: "lxd".to_string(), +/// state: "Configured".to_string(), +/// instance_ip: Some(IpAddr::V4(Ipv4Addr::new(10, 140, 190, 39))), +/// created_at: Utc.with_ymd_and_hms(2026, 2, 20, 10, 0, 0).unwrap(), +/// }; +/// +/// let output = TextView::render(&details); +/// assert!(output.contains("Environment Details:")); +/// assert!(output.contains("my-env")); +/// ``` +pub struct TextView; + +impl TextView { + /// Render configure details as human-readable formatted text + /// + /// Takes configure details and produces a human-readable output + /// suitable for displaying to users via stdout. + /// + /// # Arguments + /// + /// * `data` - Configure details to render + /// + /// # Returns + /// + /// A formatted string containing: + /// - Environment Details section with name, instance, provider, state + /// - Instance IP (if available) + /// - Creation timestamp + /// + /// # Format + /// + /// The output follows this structure: + /// ```text + /// Environment Details: + /// Name: + /// Instance: + /// Provider: + /// State: + /// Instance IP: + /// Created: + /// ``` + /// + /// # Examples + /// + /// ```rust + /// use torrust_tracker_deployer_lib::presentation::views::commands::configure::{ + /// ConfigureDetailsData, TextView, + /// }; + /// use chrono::{TimeZone, Utc}; + /// use std::net::{IpAddr, Ipv4Addr}; + /// + /// let details = ConfigureDetailsData { + /// environment_name: "prod-tracker".to_string(), + /// instance_name: "torrust-tracker-vm-prod-tracker".to_string(), + /// provider: "lxd".to_string(), + /// state: "Configured".to_string(), + /// instance_ip: Some(IpAddr::V4(Ipv4Addr::new(192, 168, 1, 100))), + /// created_at: Utc.with_ymd_and_hms(2026, 1, 5, 10, 30, 0).unwrap(), + /// }; + /// + /// let text = TextView::render(&details); + /// + /// assert!(text.contains("Name:")); + /// assert!(text.contains("prod-tracker")); + /// assert!(text.contains("Configured")); + /// ``` + #[must_use] + pub fn render(data: &ConfigureDetailsData) -> String { + let instance_ip = data + .instance_ip + .map_or_else(|| "Not available".to_string(), |ip| ip.to_string()); + + format!( + r"Environment Details: + Name: {} + Instance: {} + Provider: {} + State: {} + Instance IP: {} + Created: {}", + data.environment_name, + data.instance_name, + data.provider, + data.state, + instance_ip, + data.created_at.format("%Y-%m-%d %H:%M:%S UTC") + ) + } +} + +#[cfg(test)] +mod tests { + use super::*; + use chrono::{DateTime, TimeZone, Utc}; + use std::net::{IpAddr, Ipv4Addr}; + + // Test fixtures and helpers + + fn create_test_timestamp() -> DateTime { + Utc.with_ymd_and_hms(2026, 2, 23, 10, 0, 0).unwrap() + } + + fn create_test_ip() -> IpAddr { + IpAddr::V4(Ipv4Addr::new(10, 140, 190, 39)) + } + + fn create_test_details_with_ip(ip: Option) -> ConfigureDetailsData { + ConfigureDetailsData { + environment_name: "test-env".to_string(), + instance_name: "torrust-tracker-vm-test-env".to_string(), + provider: "lxd".to_string(), + state: "Configured".to_string(), + instance_ip: ip, + created_at: create_test_timestamp(), + } + } + + /// Helper to assert text contains all expected substrings + fn assert_contains_all(text: &str, expected: &[&str]) { + for substring in expected { + assert!( + text.contains(substring), + "Expected text to contain '{substring}' but it didn't.\nActual text:\n{text}" + ); + } + } + + // Tests + + #[test] + fn it_should_render_configure_details_as_formatted_text() { + // Arrange + let details = create_test_details_with_ip(Some(create_test_ip())); + + // Act + let text = TextView::render(&details); + + // Assert + assert_contains_all( + &text, + &[ + "Environment Details:", + "Name:", + "test-env", + "Instance:", + "torrust-tracker-vm-test-env", + "Provider:", + "lxd", + "State:", + "Configured", + "Instance IP:", + "10.140.190.39", + "Created:", + "2026-02-23 10:00:00 UTC", + ], + ); + } + + #[test] + fn it_should_display_not_available_when_instance_ip_is_none() { + // Arrange + let details = create_test_details_with_ip(None); + + // Act + let text = TextView::render(&details); + + // Assert + assert!(text.contains("Instance IP: Not available")); + } + + #[test] + fn it_should_include_all_required_sections() { + // Arrange + let details = create_test_details_with_ip(Some(IpAddr::V4(Ipv4Addr::new(192, 168, 1, 42)))); + + // Act + let text = TextView::render(&details); + + // Assert - check all sections are present + assert_contains_all( + &text, + &[ + "Name:", + "Instance:", + "Provider:", + "State:", + "Instance IP:", + "Created:", + ], + ); + } +} diff --git a/src/presentation/views/commands/mod.rs b/src/presentation/views/commands/mod.rs index 91f03acc3..3a1234ee6 100644 --- a/src/presentation/views/commands/mod.rs +++ b/src/presentation/views/commands/mod.rs @@ -4,6 +4,7 @@ //! Each command has its own submodule with views for rendering //! command-specific output. +pub mod configure; pub mod create; pub mod list; pub mod provision;