removed serial testing library.#536
removed serial testing library.#536stuartsessions wants to merge 0 commit intogit-ai-project:mainfrom
Conversation
|
|
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
src/git/test_utils/tmp_repo.rs
Outdated
| let mut env_vars = HashMap::new(); | ||
| // Set test database path if not already set (for in-process unit tests) | ||
| // Store in local env_vars instead of global environment to avoid process-wide side effects | ||
|
|
||
| // Check if it's already in the process environment | ||
| if let Ok(db_path) = std::env::var("GIT_AI_TEST_DB_PATH") { | ||
| env_vars.insert("GIT_AI_TEST_DB_PATH".to_string(), db_path); | ||
| } else { | ||
| // Create a default path and store in local env_vars | ||
| let test_db_path = std::env::temp_dir().join("git-ai-unit-test-db"); | ||
| env_vars.insert( | ||
| "GIT_AI_TEST_DB_PATH".to_string(), | ||
| test_db_path.to_string_lossy().to_string(), | ||
| ); | ||
| } |
There was a problem hiding this comment.
🔴 TmpRepo no longer sets GIT_AI_TEST_DB_PATH in the process environment, causing unit tests to hit the production database
The old TmpRepo::new() used unsafe { std::env::set_var("GIT_AI_TEST_DB_PATH", &test_db_path); } to set the env var in the process environment. The new code only stores it in the per-repo env_vars HashMap, which is only consumed by create_git_command() for spawning child processes.
Root Cause and Impact
TmpRepo is used for in-process unit tests (not subprocess-based integration tests). Functions called by these tests — commit_with_message() → post_commit() and trigger_checkpoint_with_author() → checkpoint() — invoke InternalDatabase::global() (src/authorship/internal_db.rs:349) which reads the env var via std::env::var("GIT_AI_TEST_DB_PATH"). Since the new code never sets this in the process environment, the database path falls back to the production path ~/.git-ai/internal/db.
// src/authorship/internal_db.rs:346-351
fn database_path() -> Result<PathBuf, GitAiError> {
#[cfg(any(test, feature = "test-support"))]
if let Ok(test_path) = std::env::var("GIT_AI_TEST_DB_PATH") {
return Ok(PathBuf::from(test_path));
}
// falls back to ~/.git-ai/internal/db (production!)
}Because InternalDatabase::global() uses OnceLock, the very first unit test to trigger it determines the path for the entire process. Every subsequent test in the same process will use that same (production) path.
There are dozens of unit tests across src/commands/checkpoint.rs, src/authorship/post_commit.rs, src/authorship/stats.rs, etc. that create a TmpRepo and call commit_with_message(), all of which depend on this env var being set.
Impact: Unit tests silently write to the real user's production database instead of an isolated test path, risking data corruption and causing non-deterministic test failures.
Prompt for agents
In src/git/test_utils/tmp_repo.rs, the TmpRepo::new() function (lines 273-313) needs to continue setting GIT_AI_TEST_DB_PATH in the process environment for in-process unit tests that call InternalDatabase::global(). The env_vars HashMap is only used for subprocess Command calls, but many unit tests call in-process functions (checkpoint, post_commit, blame) that read std::env::var("GIT_AI_TEST_DB_PATH") directly. Restore the unsafe std::env::set_var call (or use a thread-local override like the TEST_ENV_OVERRIDES approach in src/mdm/utils.rs, and update InternalDatabase::database_path() at src/authorship/internal_db.rs:349 to consult env_test_proxy instead of std::env::var). The simplest fix is to keep the original unsafe set_var behavior alongside the new env_vars storage, since unit tests already ran with that behavior.
Was this helpful? React with 👍 or 👎 to provide feedback.
|
The code comparison is functionally the same to what was pre-existing: if a process variable is found, use that. Otherwise, use temp_dir(). The unsafe variable set was in a conditional statement that only got reached if there wasn't an existing env var. The "GIT_AI_TEST_DB_PATH" is a test var, so shouldn't be set in user's code. |
|
Hey @stuartsessions thanks for the contribution! Watching the tests now -- is it possible that things actually got slower? |
|
Entirely possible. It definitely didn't really speed up the integration test workflow, but running unit and integration tests separately should speed it up a little. It could also be that the overhead of doing a build step separate from the unit/integration testing introduced overhead which makes it slower now... I tested the changes locally using |
|
I changed my main branch to work on a different issue not realizing it caused the PR to close. I can reopen this, just need to finagle my fork. |
|
Hey @stuartsessions thanks for checking in! Probably not honestly... We've also got HUGE updates to testing that were recently added to main and then there's even more coming in #530 with the hooks feature. Maybe after #530 lands, we could add some logic to split windows tests into 2 so that they could run in parallel? |
3 participants
PR: Make tests parallel-safe and remove
serial_testFixes #264
Summary
src/mdm/utils.rs.TestRepoenvironment injection (env_vars,add_env,set_envs,clear_envs) totests/repos/test_repo.rsso spawned child processes receive test-specific envs.env_test_proxy) where tests previously mutatedstd::env(notably opencode + checkpoint presets and prompt utilities).std::env::set_varusages in several tests with thread-local overrides (set_test_env_override) or per-repo env injection.serial_testdev-dependency and#[serial]usage from tests.--contentsmode: mapNot Committed Yet→External file (--contents)to stabilize tests.Why
std::env.Files changed (high level)
src/mdm/utils.rs— addedTEST_HOME_OVERRIDE,TEST_ENV_OVERRIDES,set_test_home_override,set_test_env_override,get_test_env_override,env_test_proxy.src/git/test_utils/tmp_repo.rs— new file extractingTmpRepoandTmpFilestructs from mod.rs with integrated unit tests for environment variable isolation.src/git/test_utils/mod.rs— cleaned up to re-exportTmpRepoandTmpFilefrom tmp_repo module; removed ~1300 lines of duplicated struct and method definitions.Cargo.toml— removedserial_testdev-dependency.src/commands/checkpoint_agent/opencode_preset.rs,src/commands/checkpoint_agent/agent_presets.rs,src/authorship/prompt_utils.rs— switched env reads to useenv_test_proxy.tests/github_copilot.rs,tests/opencode.rs,src/mdm/agents/codex.rs, and others to use test overrides instead ofstd::env::set_var.Workflow Changes
.github/workflows/tests.yml- changed the testing tool to use 'cargo-nextest' for improved performance on integration tests. changed test workflow to perform unit and integration tests concurrently.Notes & follow-ups
TmpRepoandTmpFilestructs into their own file (src/git/test_utils/tmp_repo.rs) to improve code organization and added integrated unit tests (test_repo_specific_env_vars,test_env_vars_isolated_per_repo) that validate per-repo environment variable isolation. This matches the pattern of per-repo env injection used inTestRepoand supports parallel test execution.