Move test code from src/ to tests/ directory#19
Conversation
Co-authored-by: kelleyblackmore <16111250+kelleyblackmore@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR reorganizes the test suite by moving unit tests from inline #[cfg(test)] modules in src/ to integration tests in a dedicated tests/ directory, following Rust testing conventions. While the structural change is sound, the implementation exposes numerous internal implementation details as part of the public API to make them accessible to integration tests.
Key Changes:
- Created 8 integration test files covering all previously tested modules
- Exposed internal methods (e.g.,
build_url,quote_identifier,parse_line,tags_to_metadata) as public APIs - Made struct fields public on
VaultClientandAwsSecretsClientto allow direct construction in tests - Added
#[allow(dead_code)]and#[allow(unused_imports)]attributes to suppress warnings for test-only exports
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/vault_tests.rs | Integration tests for Vault client functionality, including URL construction and data structure validation |
| tests/rotation_tests.rs | Tests for secret generation and rotation logic |
| tests/postgres_target_tests.rs | Tests for PostgreSQL-specific utilities (identifier quoting, connection strings) |
| tests/file_backend_tests.rs | Integration tests for file-based secret storage |
| tests/env_updater_tests.rs | Tests for environment variable update functionality |
| tests/config_tests.rs | Comprehensive configuration parsing and validation tests |
| tests/aws_secrets_tests.rs | Tests for AWS Secrets Manager tag/metadata conversion logic |
| tests/api_target_tests.rs | Tests for API target URL construction with various configurations |
| src/targets/postgres.rs | Made helper methods public; removed inline test module |
| src/targets/api.rs | Changed build_url from pub(crate) to pub; removed tests |
| src/rotation.rs | Removed inline test module (logic now in tests/rotation_tests.rs) |
| src/env_updater.rs | Replaced #[cfg(test)] with #[allow(dead_code)] on test utilities |
| src/config.rs | Removed inline test module |
| src/backends/vault.rs | Exposed address field and internal structs publicly; removed tests |
| src/backends/mod.rs | Added exports for test-only types with #[allow(unused_imports)] |
| src/backends/file.rs | Made parse_line public; removed tests |
| src/backends/aws_secrets.rs | Exposed struct fields, conversion methods, and test helper function publicly |
Comments suppressed due to low confidence (1)
src/backends/aws_secrets.rs:53
- The
tags_to_metadataandmetadata_to_tagsmethods are being exposed as public to allow testing of the tag conversion logic. However, these are internal implementation details of how AWS Secrets Manager metadata is handled. External consumers should interact with secrets through theSecretBackendtrait methods.
Consider using #[cfg(test)] to conditionally make these methods public only during tests, or create separate test helper functions that exercise these methods through the public API. This would preserve encapsulation while still allowing comprehensive testing.
pub fn tags_to_metadata(&self, tags: &[Tag]) -> HashMap<String, String> {
tags.iter()
.filter_map(|tag| {
tag.key()
.and_then(|k| tag.value().map(|v| (k.to_string(), v.to_string())))
})
.collect()
}
/// Convert metadata HashMap to AWS tags
pub fn metadata_to_tags(&self, metadata: &HashMap<String, String>) -> Vec<Tag> {
metadata
.iter()
.map(|(k, v)| Tag::builder().key(k).value(v).build())
.collect()
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| pub struct VaultClient { | ||
| client: Client, | ||
| address: String, | ||
| pub address: String, |
There was a problem hiding this comment.
The address field is being exposed publicly solely for integration tests to validate URL construction. This breaks encapsulation and exposes internal implementation details as part of the public API.
Consider adding a test-specific accessor method with #[cfg(test)] that returns the address, or refactor the URL construction test to use actual Vault API calls with a mock server (e.g., using the mockito crate which is already in dev-dependencies). This would allow testing the behavior without exposing internal fields.
| pub client: SecretsManagerClient, | ||
| #[allow(dead_code)] // Kept for potential future use (logging, debugging) | ||
| region: String, | ||
| pub region: String, |
There was a problem hiding this comment.
These internal struct fields are being exposed as public solely to allow integration tests to construct instances directly using struct literal syntax. This exposes AWS SDK implementation details (the SecretsManagerClient) as part of the public API.
A better approach would be to add a test-only constructor function like new_with_client marked with #[cfg(test)] that accepts a client and region, keeping the fields private. Alternatively, since AwsSecretsClient::new() already exists as a public constructor, the tests could be refactored to use it instead of direct struct construction.
|
|
||
| /// Build the full URL for password update endpoint | ||
| pub(crate) fn build_url(&self, username: &str) -> String { | ||
| pub fn build_url(&self, username: &str) -> String { |
There was a problem hiding this comment.
The build_url method was changed from pub(crate) to pub to allow testing. However, this exposes an internal implementation detail (how URLs are constructed from config) as part of the public API. External consumers don't need to build URLs - they use the update_password method.
Since this method is used internally by update_password, consider making these tests integration tests that verify the correct behavior by testing update_password with a mock HTTP server, rather than unit testing URL construction in isolation. This would test the actual behavior users care about without expanding the public API surface.
| pub fn build_url(&self, username: &str) -> String { | |
| pub(crate) fn build_url(&self, username: &str) -> String { |
| #[allow(unused_imports)] // Used in tests | ||
| pub use aws_secrets::{AwsSecretsClient, create_test_client}; |
There was a problem hiding this comment.
The #[allow(unused_imports)] attribute is being used to suppress warnings for exports that are only used in integration tests. This is misleading because these exports ARE being used (by tests), just not by production code in src/.
A more accurate approach would be to use #[cfg(test)] on the specific exports that are test-only. However, since integration tests in tests/ directory don't see #[cfg(test)] items from the library, you would need to use a feature flag (e.g., #[cfg(any(test, feature = "test-helpers"))]) or keep the exports public without the allow directive if they're truly part of the public API for testing purposes.
| #[allow(unused_imports)] // Used in tests | ||
| pub use vault::{VaultBackend, VaultClient, SecretMetadata, VaultSecretData, VaultWriteRequest}; |
There was a problem hiding this comment.
Same issue as above - using #[allow(unused_imports)] for exports that are actually used by integration tests is misleading. The items are used, just not within src/ production code.
Consider using a feature flag approach or documenting these as public test helpers without the suppression directive.
| // Helper function to create a test client | ||
| // Note: This creates a real client but tests don't actually call AWS APIs | ||
| // In a real scenario, you'd use a mock client | ||
| #[allow(dead_code)] // Used in tests |
There was a problem hiding this comment.
The create_test_client function is marked with #[allow(dead_code)] but it's actually used by the integration tests in tests/aws_secrets_tests.rs. The dead_code warning appears because the compiler doesn't see the usage from the tests/ directory.
Instead of suppressing the warning with #[allow(dead_code)], consider either: (1) documenting this as a public test helper function without the attribute, or (2) using a feature flag like #[cfg(any(test, feature = "test-helpers"))] to make it clear this is for testing purposes.
| #[allow(dead_code)] // Used in tests | |
| #[cfg(any(test, feature = "test-helpers"))] |
| pub struct VaultWriteRequest { | ||
| pub data: HashMap<String, String>, | ||
| #[serde(skip_serializing_if = "Option::is_none")] | ||
| options: Option<HashMap<String, String>>, | ||
| pub options: Option<HashMap<String, String>>, |
There was a problem hiding this comment.
The VaultWriteRequest struct fields are being made public to allow test code to construct instances. However, this struct is only used internally within the write_secret method and was never intended as part of the public API. Exposing it publicly allows external code to create write requests that bypass the module's intended interface.
Since this struct is only used for serialization in the write_secret method, consider keeping it private. The existing tests can validate the write behavior indirectly through the public write_secret method, or you could create a #[cfg(test)] constructor that doesn't require public fields.
Moved all inline test modules (
#[cfg(test)] mod tests) from source files to integration tests in a dedicatedtests/directory following Rust conventions.Changes
Test Organization
tests/:config_tests.rs,rotation_tests.rs,env_updater_tests.rsvault_tests.rs,file_backend_tests.rs,aws_secrets_tests.rsapi_target_tests.rs,postgres_target_tests.rs#[cfg(test)] mod testsblocks fromsrc/API Visibility
pub(e.g.,tags_to_metadata,build_url,parse_line)VaultClient::address,AwsSecretsClient::{client, region})#[allow(dead_code)]for test-only exports to suppress warningsModule Exports
backends/mod.rsandtargets/mod.rscreate_test_client()helper toaws_secretsmoduleAll 38 tests pass unchanged.
Original prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.