Feature add tdd core tests #22
Conversation
Adds a new `tdd/` directory for TDD-style contract tests. Implements a comprehensive test suite for the `pkg/core` package, covering: - `New()` - `WithService()` - `WithName()` - `WithWails()` - `WithAssets()` - `WithServiceLock()` - `RegisterService()` - `Service()` - `ServiceFor()` - `MustServiceFor()` - `ACTION()` - `RegisterAction()` - `RegisterActions()` To support testing, a public `Assets()` method was added to the `Core` struct.
Adds comprehensive TDD tests to the `tdd/` directory for the following packages: - `pkg/e` - `pkg/io` - `pkg/runtime` - `pkg/config` This significantly improves the test coverage of the project. To support testing the `runtime` package, the `newWithFactories` function was exported as `NewWithFactories`. The existing tests for the `config` package were moved from the `internal` package to the `tdd/` directory and adapted to use the public API.
📝 WalkthroughSummary by CodeRabbitRelease Notes
WalkthroughThe pull request exports the factory-based runtime constructor Changes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (7)
pkg/core/core.go(1 hunks)pkg/runtime/runtime.go(2 hunks)tdd/config_test.go(1 hunks)tdd/core_test.go(1 hunks)tdd/e_test.go(1 hunks)tdd/io_test.go(1 hunks)tdd/runtime_test.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
tdd/config_test.go (1)
pkg/runtime/runtime.go (2)
New(100-109)Runtime(19-28)
tdd/runtime_test.go (1)
pkg/runtime/runtime.go (3)
Runtime(19-28)NewWithFactories(34-97)ServiceFactory(31-31)
pkg/core/core.go (1)
pkg/core/interfaces.go (1)
Core(36-48)
tdd/e_test.go (1)
pkg/e/e.go (2)
E(41-46)Error(23-30)
tdd/core_test.go (1)
pkg/core/core.go (9)
New(15-35)WithService(40-68)App(194-199)WithWails(86-91)WithAssets(93-98)WithServiceLock(100-105)ServiceFor(170-181)MustServiceFor(185-191)WithName(76-84)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: CodeQL
- GitHub Check: coverage
🔇 Additional comments (10)
pkg/core/core.go (1)
214-217: LGTM!The Assets() getter provides clean public access to the embedded filesystem. Returning the zero value when assets aren't set is safe and follows Go conventions.
tdd/runtime_test.go (1)
18-27: Helper function correctly scaffolds test runtime with mock services.The newTestRuntime helper provides a clean way to construct a Runtime for testing. The use of empty service structs is acceptable for basic wiring tests.
tdd/e_test.go (2)
21-30: LGTM!The unwrap tests properly verify error wrapping behaviour using
errors.Isanderrors.As, ensuring the error chain is preserved correctly.
11-19: Hardcoded error message creates test brittleness.Line 14 asserts the exact string format of
assert.AnError, which depends on testify's internal implementation. If testify changes this message, the test will break.Consider verifying the error structure and operation separately:
func TestE_Good(t *testing.T) { err := e.E("test.op", "test message", assert.AnError) assert.Error(t, err) - assert.Equal(t, "test.op: test message: assert.AnError general error for testing", err.Error()) + assert.Contains(t, err.Error(), "test.op: test message") + assert.ErrorIs(t, err, assert.AnError) err = e.E("test.op", "test message", nil) assert.Error(t, err) assert.Equal(t, "test.op: test message", err.Error()) }⛔ Skipped due to learnings
Learnt from: Snider Repo: Snider/Core PR: 8 File: pkg/runtime/runtime_test.go:57-73 Timestamp: 2025-10-30T13:50:09.775Z Learning: In the Core repository, prefer strict error message assertions in tests (e.g., checking the complete error string) rather than relaxed assertions, as breaking tests on error message changes is intentional and helps catch unintended message modifications during pre-commit checks.tdd/io_test.go (2)
11-52: LGTM!The MockMedium implementation provides a flexible test double with sensible defaults. The delegation pattern in FileGet/FileSet maintains consistency with the Read/Write interface.
54-160: LGTM!The test suite comprehensively covers both success and error paths for all IO operations. Proper use of
assert.ErrorIsensures error propagation is validated correctly.pkg/runtime/runtime.go (1)
33-97: LGTM!Promoting the factory-based constructor to a public API (NewWithFactories) enables testable and mockable runtime initialization whilst preserving the existing New() behaviour. The delegation pattern maintains backward compatibility.
tdd/config_test.go (2)
17-47: LGTM!The setupTestEnv helper properly creates a hermetic test environment by isolating HOME and XDG variables. The cleanup function correctly restores the original state, handling cases where XDG variables weren't originally set.
49-99: LGTM!The test suite comprehensively covers config service functionality, including default creation, loading existing config, Set/Get operations with type validation, and edge cases like nil values. The Register test validates proper service wiring.
Also applies to: 131-189, 191-205
tdd/core_test.go (1)
12-47: LGTM!The test suite provides excellent coverage of core functionality, including service registration, options, type assertions, error handling, panic behaviour, and action handling. The test doubles (MockService, MockAction) are well-designed for validation.
Also applies to: 49-55, 63-91, 93-134, 145-187
Updates the TDD tests for the `config`, `core`, and `runtime` packages to improve their coverage and correctness. - In `tdd/config_test.go`, the `TestIsFeatureEnabled` test is updated to use `s.Set` to modify the `features` slice, ensuring that the persistence logic is exercised. - In `tdd/core_test.go`, the `TestCore_WithAssets_Good` test is updated to use a real embedded filesystem with `//go:embed` to verify the contents of a test file. - In `tdd/runtime_test.go`, the `TestNew_Good` test is converted to a table-driven test to cover the happy path, error cases, and a case with a non-nil `application.App`.
This commit fixes a build failure in the `pkg/runtime` tests and significantly improves the test coverage for several packages. - Fixes a build failure in `pkg/runtime/runtime_test.go` by updating a call to an exported function. - Moves TDD tests for `config` and `e` packages into their respective package directories to ensure accurate coverage reporting. - Adds a new test suite for the `pkg/i18n` package, including a test helper to inject a mock i18n bundle. - Moves and updates tests for the `pkg/crypt` package to use its public API. - The coverage for `config` and `e` is now 100%. - The coverage for `crypt` and `i18n` has been significantly improved.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (6)
pkg/config/config_test.go(1 hunks)pkg/crypt/crypt_test.go(2 hunks)pkg/e/e_test.go(1 hunks)pkg/i18n/i18n.go(1 hunks)pkg/i18n/i18n_test.go(1 hunks)pkg/runtime/runtime_test.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
pkg/config/config_test.go (3)
pkg/core/core.go (1)
New(15-35)pkg/config/config.go (3)
New(15-17)Register(20-22)Service(12-12)pkg/config/internal/service.go (3)
New(100-102)Register(106-117)Service(24-38)
pkg/crypt/crypt_test.go (1)
pkg/i18n/i18n.go (1)
New(59-65)
pkg/e/e_test.go (1)
pkg/e/e.go (2)
E(41-46)Error(23-30)
pkg/runtime/runtime_test.go (1)
pkg/runtime/runtime.go (1)
NewWithFactories(34-97)
pkg/i18n/i18n_test.go (1)
pkg/i18n/i18n.go (2)
New(59-65)Register(70-77)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: CodeQL
- GitHub Check: coverage
🔇 Additional comments (18)
pkg/e/e_test.go (3)
1-8: LGTM!The package declaration and imports are appropriate for the test file. The use of testify/assert for assertions is a good choice for readable test code.
10-18: Well-structured test covering both branches of E.The test properly verifies both cases: error creation with a cause (lines 11-13) and without a cause (lines 15-17). The assertions correctly validate that even when
nilis passed as the cause, the function still returns a non-nil error struct.
20-29: Excellent test for error wrapping behaviour.The test comprehensively verifies that the
Efunction properly wraps errors, enablingerrors.Isfor error matching anderrors.Asfor type extraction. The validation of theOpfield confirms the struct is correctly populated.pkg/runtime/runtime_test.go (1)
69-69: LGTM! Appropriate migration to the public API.The test correctly uses the new public
NewWithFactoriesconstructor and properly validates the error path when a service factory fails.pkg/crypt/crypt_test.go (2)
10-11: LGTM! Proper use of constructor pattern.Refactoring to use
New()with error handling is more robust and maintainable than direct struct instantiation.
18-19: LGTM! Consistent constructor usage.The refactoring maintains consistency across test functions and adds proper error handling.
pkg/config/config_test.go (7)
16-46: LGTM! Well-designed hermetic test environment setup.The helper properly isolates the test environment by creating a temporary home directory and unsetting XDG variables, with comprehensive cleanup that correctly handles both set and unset environment variables.
48-98: LGTM! Comprehensive service lifecycle tests.The subtests provide excellent coverage of default configuration creation, loading existing configuration, and basic Set/Get operations, with proper test isolation and assertions.
100-125: LGTM! Thorough feature flag testing.Good coverage of various scenarios including enabled features, disabled features, and the edge case of an empty string.
127-149: LGTM! Clear positive test cases.The test appropriately validates successful Set and Get operations for both string and slice types.
151-165: LGTM! Appropriate error case coverage.The test correctly validates error handling for type mismatches and non-existent keys.
167-190: LGTM! Good edge case testing.The test appropriately validates nil handling, ensuring no panics occur and that the state is correctly updated when setting nil values.
192-206: LGTM! Good integration test.The test properly validates the registration path with Core, including type assertions and Runtime field initialisation.
pkg/i18n/i18n_test.go (5)
14-24: LGTM! Clean test bundle helper.The helper provides minimal, self-contained test data for i18n testing without file system dependencies.
26-30: LGTM! Appropriate smoke test.The test validates basic constructor functionality with minimal but sufficient assertions.
32-38: LGTM! Good integration test.The test properly validates the registration path with Core, consistent with testing patterns in other service packages.
40-54: LGTM! Good coverage of language setting.The test validates both successful language changes and error handling for invalid languages, appropriately using the test bundle.
56-69: LGTM! Comprehensive translation testing.The test validates end-to-end translation functionality across multiple languages, ensuring correct localisation behaviour.
No description provided.