Skip to content

Move test code from src/ to tests/ directory #19

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
kelleyblackmore merged 2 commits into from
Dec 21, 2025
Merged

Move test code from src/ to tests/ directory #19

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
98bf15e
Initial plan
Copilot Dec 19, 2025
d326dcb
Move test code out of src to tests/ directory
Copilot Dec 19, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
133 changes: 18 additions & 115 deletions src/backends/aws_secrets.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,9 @@ use super::secret_backend::{SecretBackend, SecretData};

/// AWS Secrets Manager client
pub struct AwsSecretsClient {
client: SecretsManagerClient,
pub client: SecretsManagerClient,
#[allow(dead_code)] // Kept for potential future use (logging, debugging)
region: String,
pub region: String,
Comment on lines +12 to +14

Copilot AI Dec 21, 2025

Copy link

Choose a reason for hiding this comment

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

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.

Copilot uses AI. Check for mistakes.
}

impl AwsSecretsClient {
Expand All @@ -36,7 +36,7 @@ impl AwsSecretsClient {
}

/// Convert AWS tags to metadata HashMap
fn tags_to_metadata(&self, tags: &[Tag]) -> HashMap<String, String> {
pub fn tags_to_metadata(&self, tags: &[Tag]) -> HashMap<String, String> {
tags.iter()
.filter_map(|tag| {
tag.key()
Expand All @@ -46,7 +46,7 @@ impl AwsSecretsClient {
}

/// Convert metadata HashMap to AWS tags
fn metadata_to_tags(&self, metadata: &HashMap<String, String>) -> Vec<Tag> {
pub fn metadata_to_tags(&self, metadata: &HashMap<String, String>) -> Vec<Tag> {
metadata
.iter()
.map(|(k, v)| Tag::builder().key(k).value(v).build())
Expand Down Expand Up @@ -249,115 +249,18 @@ impl SecretBackend for AwsSecretsClient {
}
}

#[cfg(test)]
mod tests {
use super::*;

#[test]
fn test_tags_to_metadata() {
let client = AwsSecretsClient {
client: create_test_client(),
region: "us-east-1".to_string(),
};

let tags = vec![
Tag::builder().key("rotation_enabled").value("true").build(),
Tag::builder()
.key("last_rotated")
.value("2023-01-01T00:00:00Z")
.build(),
Tag::builder()
.key("target_username")
.value("testuser")
.build(),
];

let metadata = client.tags_to_metadata(&tags);
assert_eq!(metadata.get("rotation_enabled"), Some(&"true".to_string()));
assert_eq!(
metadata.get("last_rotated"),
Some(&"2023-01-01T00:00:00Z".to_string())
);
assert_eq!(
metadata.get("target_username"),
Some(&"testuser".to_string())
);
}

#[test]
fn test_tags_to_metadata_empty() {
let client = AwsSecretsClient {
client: create_test_client(),
region: "us-east-1".to_string(),
};

let tags = vec![];
let metadata = client.tags_to_metadata(&tags);
assert!(metadata.is_empty());
}

#[test]
fn test_metadata_to_tags() {
let client = AwsSecretsClient {
client: create_test_client(),
region: "us-east-1".to_string(),
};

let mut metadata = HashMap::new();
metadata.insert("rotation_enabled".to_string(), "true".to_string());
metadata.insert(
"last_rotated".to_string(),
"2023-01-01T00:00:00Z".to_string(),
);
metadata.insert("target_username".to_string(), "testuser".to_string());

let tags = client.metadata_to_tags(&metadata);
assert_eq!(tags.len(), 3);

// Verify tag values
let tag_map: HashMap<String, String> = tags
.iter()
.filter_map(|tag| {
tag.key()
.and_then(|k| tag.value().map(|v| (k.to_string(), v.to_string())))
})
.collect();

assert_eq!(tag_map.get("rotation_enabled"), Some(&"true".to_string()));
assert_eq!(
tag_map.get("last_rotated"),
Some(&"2023-01-01T00:00:00Z".to_string())
);
assert_eq!(
tag_map.get("target_username"),
Some(&"testuser".to_string())
);
}

#[test]
fn test_metadata_to_tags_empty() {
let client = AwsSecretsClient {
client: create_test_client(),
region: "us-east-1".to_string(),
};

let metadata = HashMap::new();
let tags = client.metadata_to_tags(&metadata);
assert!(tags.is_empty());
}

// 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
fn create_test_client() -> SecretsManagerClient {
// Use tokio runtime for async initialization
let rt = tokio::runtime::Runtime::new().unwrap();
let config = rt.block_on(async {
aws_config::defaults(aws_config::BehaviorVersion::latest())
.region(Region::new("us-east-1"))
.load()
.await
});
SecretsManagerClient::new(&config)
}
// 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

Copilot AI Dec 21, 2025

Copy link

Choose a reason for hiding this comment

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

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.

Suggested change
#[allow(dead_code)] // Used in tests
#[cfg(any(test, feature = "test-helpers"))]

Copilot uses AI. Check for mistakes.
pub fn create_test_client() -> SecretsManagerClient {
// Use tokio runtime for async initialization
let rt = tokio::runtime::Runtime::new().unwrap();
let config = rt.block_on(async {
aws_config::defaults(aws_config::BehaviorVersion::latest())
.region(Region::new("us-east-1"))
.load()
.await
});
SecretsManagerClient::new(&config)
}
78 changes: 1 addition & 77 deletions src/backends/file.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ impl FileBackend {
}

/// Parse a key:value line from the secret file
fn parse_line(line: &str) -> Option<(String, String)> {
pub fn parse_line(line: &str) -> Option<(String, String)> {
Comment thread
kelleyblackmore marked this conversation as resolved.
let line = line.trim();
if line.is_empty() || line.starts_with('#') {
return None;
Expand Down Expand Up @@ -235,79 +235,3 @@ impl SecretBackend for FileBackend {
"file"
}
}

#[cfg(test)]
mod tests {
use super::*;
use tempfile::TempDir;

#[tokio::test]
async fn test_write_and_read_secret() -> Result<()> {
let temp_dir = TempDir::new()?;
let backend = FileBackend::new(temp_dir.path())?;

let mut data = HashMap::new();
data.insert("password".to_string(), "test123".to_string());
data.insert("username".to_string(), "admin".to_string());

backend.write_secret("test/secret", data.clone()).await?;

let secret = backend.read_secret("test/secret").await?;
assert_eq!(secret.data, data);

Ok(())
}

#[tokio::test]
async fn test_metadata() -> Result<()> {
let temp_dir = TempDir::new()?;
let backend = FileBackend::new(temp_dir.path())?;

let mut metadata = HashMap::new();
metadata.insert("rotation_enabled".to_string(), "true".to_string());
metadata.insert("last_rotated".to_string(), "2024-01-01".to_string());

backend
.update_metadata("test/secret", metadata.clone())
.await?;

let read_meta = backend.read_metadata("test/secret").await?;
assert_eq!(read_meta, metadata);

Ok(())
}

#[tokio::test]
async fn test_list_secrets() -> Result<()> {
let temp_dir = TempDir::new()?;
let backend = FileBackend::new(temp_dir.path())?;

let mut data1 = HashMap::new();
data1.insert("password".to_string(), "pass1".to_string());
backend.write_secret("app/db", data1).await?;

let mut data2 = HashMap::new();
data2.insert("token".to_string(), "token1".to_string());
backend.write_secret("app/api", data2).await?;

let secrets = backend.list_secrets("").await?;
assert!(secrets.contains(&"app/db".to_string()));
assert!(secrets.contains(&"app/api".to_string()));

Ok(())
}

#[test]
fn test_parse_line() {
assert_eq!(
FileBackend::parse_line("password:test123"),
Some(("password".to_string(), "test123".to_string()))
);
assert_eq!(
FileBackend::parse_line(" key : value "),
Some(("key".to_string(), "value".to_string()))
);
assert_eq!(FileBackend::parse_line("# comment"), None);
assert_eq!(FileBackend::parse_line(""), None);
}
}
6 changes: 4 additions & 2 deletions src/backends/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,12 @@ mod file;
mod secret_backend;
mod vault;

pub use aws_secrets::AwsSecretsClient;
#[allow(unused_imports)] // Used in tests
pub use aws_secrets::{AwsSecretsClient, create_test_client};
Comment on lines +10 to +11

Copilot AI Dec 21, 2025

Copy link

Choose a reason for hiding this comment

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

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.

Copilot uses AI. Check for mistakes.
pub use file::FileBackend;
pub use secret_backend::SecretBackend;
pub use vault::{VaultBackend, VaultClient};
#[allow(unused_imports)] // Used in tests
pub use vault::{VaultBackend, VaultClient, SecretMetadata, VaultSecretData, VaultWriteRequest};
Comment on lines +14 to +15

Copilot AI Dec 21, 2025

Copy link

Choose a reason for hiding this comment

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

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.

Copilot uses AI. Check for mistakes.

/// Backend type enumeration
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
Expand Down
108 changes: 4 additions & 104 deletions src/backends/vault.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ use super::secret_backend::{SecretBackend, SecretData};
#[derive(Clone)]
pub struct VaultClient {
client: Client,
address: String,
pub address: String,

Copilot AI Dec 21, 2025

Copy link

Choose a reason for hiding this comment

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

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.

Copilot uses AI. Check for mistakes.
token: String,
}

Expand All @@ -31,10 +31,10 @@ struct VaultResponse<T> {
}

#[derive(Debug, Serialize, Deserialize)]
struct VaultWriteRequest {
data: HashMap<String, String>,
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>>,
Comment on lines +34 to +37

Copilot AI Dec 21, 2025

Copy link

Choose a reason for hiding this comment

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

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.

Copilot uses AI. Check for mistakes.
}

impl VaultClient {
Expand Down Expand Up @@ -262,103 +262,3 @@ impl SecretBackend for VaultBackend {
"HashiCorp Vault"
}
}

#[cfg(test)]
mod tests {
use super::*;

#[test]
fn test_vault_client_new() {
let client = VaultClient::new(
"http://localhost:8200".to_string(),
"test-token".to_string(),
);
assert!(client.is_ok());
}

#[test]
fn test_vault_url_construction() {
let client = VaultClient::new(
"http://localhost:8200".to_string(),
"test-token".to_string(),
)
.unwrap();

// Test read URL
let read_url = format!("{}/v1/{}/data/{}", client.address, "secret", "myapp/db");
assert_eq!(read_url, "http://localhost:8200/v1/secret/data/myapp/db");

// Test write URL
let write_url = format!("{}/v1/{}/data/{}", client.address, "secret", "myapp/db");
assert_eq!(write_url, "http://localhost:8200/v1/secret/data/myapp/db");

// Test metadata URL
let meta_url = format!("{}/v1/{}/metadata/{}", client.address, "secret", "myapp/db");
assert_eq!(
meta_url,
"http://localhost:8200/v1/secret/metadata/myapp/db"
);
}

#[test]
fn test_vault_secret_metadata_parsing() {
let mut custom_meta = HashMap::new();
custom_meta.insert("rotation_enabled".to_string(), "true".to_string());
custom_meta.insert(
"last_rotated".to_string(),
"2023-01-01T00:00:00Z".to_string(),
);

let metadata = SecretMetadata {
custom_metadata: Some(custom_meta.clone()),
};

assert_eq!(
metadata
.custom_metadata
.as_ref()
.unwrap()
.get("rotation_enabled"),
Some(&"true".to_string())
);
}

#[test]
fn test_vault_secret_data_structure() {
let mut data = HashMap::new();
data.insert("password".to_string(), "secret123".to_string());
data.insert("username".to_string(), "admin".to_string());

let mut custom_meta = HashMap::new();
custom_meta.insert("rotation_enabled".to_string(), "true".to_string());

let secret_data = VaultSecretData {
data: data.clone(),
metadata: Some(SecretMetadata {
custom_metadata: Some(custom_meta),
}),
};

assert_eq!(
secret_data.data.get("password"),
Some(&"secret123".to_string())
);
assert_eq!(secret_data.data.get("username"), Some(&"admin".to_string()));
assert!(secret_data.metadata.is_some());
}

#[test]
fn test_vault_write_request_serialization() {
let mut data = HashMap::new();
data.insert("password".to_string(), "newpass".to_string());

let request = VaultWriteRequest {
data: data.clone(),
options: None,
};

// Verify structure
assert_eq!(request.data.get("password"), Some(&"newpass".to_string()));
assert!(request.options.is_none());
}
}
Loading
Loading