Skip to content

Refactor env guard and fix compilation #40

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
leynos merged 7 commits into from
Aug 3, 2025
Merged

Refactor env guard and fix compilation #40

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
e1960c4
Refactor env guard and fix compilation
leynos Aug 2, 2025
9d2eef2
Remove unused rstest imports
leynos Aug 3, 2025
47fbcdb
Align test deps and tag validation
leynos Aug 3, 2025
032a791
Update lock file
leynos Aug 3, 2025
b20f9aa
Use correct version pattern
leynos Aug 3, 2025
a86e272
Remove unneeded import
leynos Aug 3, 2025
b33662e
Remove unused UnixListener import
leynos Aug 3, 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
2 changes: 1 addition & 1 deletion .github/workflows/release.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ on:
push:
tags:
# Match semantic version tags (e.g. v1.2.3, v10.11.12, v12.3.7-beta7)
- 'v[0-9]*.[0-9]*.[0-9]*'
- 'v*.*.*'

jobs:
goreleaser:
Expand Down
4 changes: 4 additions & 0 deletions Cargo.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

7 changes: 5 additions & 2 deletions Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,11 +18,12 @@ clap = { workspace = true }
comenq = { path = "crates/comenq" }
comenqd = { path = "crates/comenqd" }
ortho_config = { git = "https://github.com/leynos/ortho-config.git", tag = "v0.4.0" }
tempfile = "3.10" # latest 3.x at time of writing; update as new patch versions release
tempfile = { workspace = true } # latest 3.x at time of writing; update as new patch versions release
yaque = { workspace = true }
wiremock = "0.6"
wiremock = { workspace = true }
octocrab = { workspace = true }
test-support = { path = "test-support" }
regex = { workspace = true }

[[test]]
name = "cucumber"
Expand Down Expand Up @@ -51,7 +52,9 @@ thiserror = "1.0"
ortho_config = { git = "https://github.com/leynos/ortho-config.git", tag = "v0.4.0" }
serde_yaml = "0.9"
tempfile = "3.10"
regex = "1"
rstest = "0.18.0"
wiremock = "0.6"

[lints.clippy]
pedantic = { level = "warn", priority = -1 }
Expand Down
4 changes: 3 additions & 1 deletion crates/comenqd/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,4 +23,6 @@ figment = { version = "0.10", default-features = false, features = ["env", "toml
rstest = { workspace = true }
tempfile = { workspace = true } # latest 3.x at time of writing; update as new patch versions release
serial_test = "2"
wiremock = "0.6"
test-support = { path = "../../test-support" }
test-utils = { path = "../test-utils" }
wiremock = { workspace = true }
48 changes: 1 addition & 47 deletions crates/comenqd/src/config.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,53 +118,7 @@ mod tests {
use std::fs;
use tempfile::tempdir;

mod env_guard {
//! Test helpers for managing environment variables.

#[derive(Debug)]
pub struct EnvVarGuard {
key: String,
original: Option<String>,
}

impl EnvVarGuard {
/// Set an environment variable for the lifetime of the returned guard.
pub fn set(key: &str, value: &str) -> Self {
let original = std::env::var(key).ok();
set_env_var(key, value);
Self {
key: key.to_string(),
original,
}
}
}

impl Drop for EnvVarGuard {
fn drop(&mut self) {
match &self.original {
Some(v) => set_env_var(&self.key, v),
None => remove_env_var(&self.key),
}
}
}

/// Set an environment variable for tests.
///
/// The nightly compiler marks `std::env::set_var` as `unsafe`.
/// Tests run serially so using it is acceptable here.
pub fn set_env_var(key: &str, value: &str) {
unsafe { std::env::set_var(key, value) };
}

/// Remove an environment variable for tests.
///
/// `std::env::remove_var` is also `unsafe` on nightly.
pub fn remove_env_var(key: &str) {
unsafe { std::env::remove_var(key) };
}
}

use env_guard::{EnvVarGuard, remove_env_var};
use test_support::env_guard::{EnvVarGuard, remove_env_var};
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot Aug 3, 2025

Choose a reason for hiding this comment

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

💡 Verification agent

🧩 Analysis chain

Import change looks good, but verify unsafe usage in shared utility.

The migration to shared test utilities improves code organisation. However, ensure the unsafe operation in test_support::env_guard::remove_env_var (which calls std::env::remove_var) is properly justified and documented according to the coding guidelines.

🏁 Script executed:

#!/bin/bash
# Verify that the unsafe usage in remove_env_var is properly documented
ast-grep --pattern $'pub fn remove_env_var($_) {
    $$$
    unsafe { $$$ }
    $$$
}'

Length of output: 265

Remove unnecessary unsafe block in remove_env_var

The remove_env_var function wraps the safe std::env::remove_var call in an unsafe block. Eliminate this block to comply with our guideline of avoiding superfluous unsafe usage.

• File: test-support/src/env_guard.rs
• Lines: 44–46

Suggested change:

- pub fn remove_env_var(key: &str) {
-     unsafe { std::env::remove_var(key) };
- }
+ pub fn remove_env_var(key: &str) {
+     std::env::remove_var(key);
+ }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
use test_support::env_guard::{EnvVarGuard, remove_env_var};
// In test-support/src/env_guard.rs
pub fn remove_env_var(key: &str) {
std::env::remove_var(key);
}
🤖 Prompt for AI Agents 
In crates/comenqd/src/config.rs at line 121, the import of remove_env_var is
noted, but the actual issue is in test-support/src/env_guard.rs lines 44 to 46
where remove_env_var wraps std::env::remove_var in an unnecessary unsafe block.
To fix this, remove the unsafe block around std::env::remove_var call in
remove_env_var, making the function fully safe and compliant with guidelines
against unnecessary unsafe usage.


#[rstest]
#[serial_test::serial]
Expand Down
3 changes: 3 additions & 0 deletions test-support/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,3 +9,6 @@ comenqd = { path = "../crates/comenqd" }
octocrab = { workspace = true }
tempfile = { workspace = true }
wiremock = "^0.6"

[dev-dependencies]
serial_test = "^2"
0 tests/support/env_guard.rs → test-support/src/env_guard.rs
View file
Open in desktop
File renamed without changes.
2 changes: 2 additions & 0 deletions test-support/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,12 @@
//! Test support utilities.

pub mod daemon;
pub mod env_guard;
pub mod util;

// Re-exports from daemon module (added in main)
pub use daemon::{octocrab_for, temp_config};
pub use env_guard::{EnvVarGuard, remove_env_var, set_env_var};

// Re-exports from util module with documentation (from your branch)
/// Maximum number of times to poll for an expected file.
Expand Down
1 change: 0 additions & 1 deletion tests/cucumber.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@
//! parallel.

mod steps;
mod support;
mod util;
use cucumber::World as _;
use steps::{
Expand Down
2 changes: 1 addition & 1 deletion tests/steps/config_steps.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,8 @@ use std::fs;
use std::path::PathBuf;
use tempfile::TempDir;

use crate::support::env_guard::{EnvVarGuard, remove_env_var};
use comenqd::config::Config;
use test_support::env_guard::{EnvVarGuard, remove_env_var};

#[derive(Debug, Default, World)]
pub struct ConfigWorld {
Expand Down
6 changes: 5 additions & 1 deletion tests/steps/release_steps.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

use comenq_lib::workflow::uses_goreleaser as workflow_uses_goreleaser;
use cucumber::{World, given, then, when};
use regex::Regex;
use serde_yaml::Value;
use std::fs;

Expand Down Expand Up @@ -40,8 +41,11 @@ fn triggers_on_tags(world: &mut ReleaseWorld) {
.expect("tags")
.as_sequence()
.expect("sequence");
let pattern = Regex::new(r"^v\*\.\*\.\*$").expect("compile regex");
assert!(
Comment thread
leynos marked this conversation as resolved.
tags.iter()
.any(|t| t.as_str() == Some("v[0-9]*.[0-9]*.[0-9]*"))
.filter_map(|t| t.as_str())
.any(|t| pattern.is_match(t)),
"missing semantic version tag pattern",
);
Comment on lines +44 to 50
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot Aug 3, 2025

Choose a reason for hiding this comment

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

⚠️ Potential issue

Fix regex pattern for semantic version matching.

The current regex pattern ^v\*\.\*\.\*$ matches the literal string "v*.." rather than semantic version tags like "v1.2.3".

Apply this diff to fix the pattern:

-    let pattern = Regex::new(r"^v\*\.\*\.\*$").expect("compile regex");
+    let pattern = Regex::new(r"^v\d+\.\d+\.\d+$").expect("compile regex");

This will properly match semantic version tags with the format "v{major}.{minor}.{patch}".

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
let pattern = Regex::new(r"^v\*\.\*\.\*$").expect("compile regex");
assert!(
tags.iter()
.any(|t| t.as_str() == Some("v[0-9]*.[0-9]*.[0-9]*"))
.filter_map(|t| t.as_str())
.any(|t| pattern.is_match(t)),
"missing semantic version tag pattern",
);
let pattern = Regex::new(r"^v\d+\.\d+\.\d+$").expect("compile regex");
assert!(
tags.iter()
.filter_map(|t| t.as_str())
.any(|t| pattern.is_match(t)),
"missing semantic version tag pattern",
);
🤖 Prompt for AI Agents 
In tests/steps/release_steps.rs around lines 44 to 50, the regex pattern used to
match semantic version tags is incorrect as it matches the literal string
"v*.*.*" instead of versions like "v1.2.3". Update the regex pattern to
correctly match semantic version tags by replacing the pattern with one that
matches a 'v' followed by digits, dots, and digits in the format
"v{major}.{minor}.{patch}".

}
3 changes: 0 additions & 3 deletions tests/support/mod.rs
View file
Open in desktop

This file was deleted.

Loading