Skip to content
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.

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

Some tests will fail if one have a local git config user.name/user.email #7469

Closed
hbina opened this issue Oct 2, 2019 · 4 comments · Fixed by #7944
Closed

Some tests will fail if one have a local git config user.name/user.email #7469

hbina opened this issue Oct 2, 2019 · 4 comments · Fixed by #7944
Labels
A-testing-cargo-itself Area: cargo's tests C-bug Category: bug

Comments

@hbina
Copy link
Contributor

hbina commented Oct 2, 2019

Problem

Running cargo test --all will fail if you have a local .git/config. Link to the log file is here https://gist.github.com/hbina/8b09995b55e95267b3286a99a30a54f2

Specifically, the following unit tests will fail.

new::finds_author_email
new::finds_author_git
new::finds_author_user
new::finds_author_user_escaped
new::finds_author_username
new::finds_git_author
new::strip_angle_bracket_author_email

Steps

  1. Clone the repository
  2. Set a local user.name and/or user.email
  3. Run test
  4. ????
  5. No profit!

Possible Solution(s)

@ehuss suggested a solution that uses the environment variable __CARGO_TEST_ROOT. The following is my attempt to use that. This code could be infinitely better, I guess. Note that this does not actually solve the problem, but some variations of them work (except for one test). I think the problem is with my logic.

diff --git a/src/cargo/ops/cargo_new.rs b/src/cargo/ops/cargo_new.rs
index 512eed7e..9ae7ba1d 100644
--- a/src/cargo/ops/cargo_new.rs
+++ b/src/cargo/ops/cargo_new.rs
@@ -698,9 +698,23 @@ fn get_environment_variable(variables: &[&str]) -> Option<String> {
 fn discover_author() -> CargoResult<(String, Option<String>)> {
     let cwd = env::current_dir()?;
     let git_config = if let Ok(repo) = GitRepository::discover(&cwd) {
-        repo.config()
-            .ok()
-            .or_else(|| GitConfig::open_default().ok())
+        let repo_path = repo.path();
+        let cargo_test_root = env::var("__CARGO_TEST_ROOT");
+        match cargo_test_root {
+            Ok(ok) => {
+                if repo_path.starts_with(ok) 

{

+                    GitConfig::open_default().ok()
+                } else {
+                    repo.config()
+                        .ok()
+                        .or_else(|| GitConfig::open_default().ok())
+                }
+            }
+            Err(_) => repo
+                .config()
+                .ok()
+                .or_else(|| GitConfig::open_default().ok()),
+        }
     } else {
         GitConfig::open_default().ok()
     };

Notes

Output of cargo version: cargo 1.38.0 (23ef9a4ef 2019-08-20)

@hbina hbina added the C-bug Category: bug label Oct 2, 2019
@mathstuf
Copy link
Contributor

mathstuf commented Oct 2, 2019

I have a local user.email set in my repo and I'm not seeing this. What version of git are you using? Can you get a strace -f -e file log of running just those tests to see what .git/config files it is using (and maybe see how that is getting picked up)?

@ehuss
Copy link
Contributor

ehuss commented Oct 3, 2019

I'm able to repro it pretty easily by running git config user.email foo@example.com to populate cargo/.git/config, then cargo test --test testsuite -- new::. It makes sense because these lines hunt for the closest repository which happens to be cargo itself, and then loads the config from there. Something akin to the given patch should probably work, though I think the starts_with logic may need to be reversed.

@hbina
Copy link
Contributor Author

hbina commented Oct 3, 2019

Can you get a strace -f -e file log of running just those tests to see what .git/config files it is using (and maybe see how that is getting picked up)?

Sorry, but I don't quite understand this instructions at all...

@mathstuf
Copy link
Contributor

mathstuf commented Oct 3, 2019

Oh, I see. I'm saved because my build tree is off in the weeds on another partition (I do this to keep build artifacts out of my backups).

@ehuss ehuss added the A-testing-cargo-itself Area: cargo's tests label Oct 10, 2019
bors added a commit that referenced this issue Feb 28, 2020
Add a special case for git config discovery inside tests

Fixes #7469: Some tests will fail if one have a local git config user.name/user.email
@bors bors closed this as completed in 0f51c48 Feb 28, 2020
bors added a commit that referenced this issue Dec 2, 2020
Fix test escaping __CARGO_TEST_ROOT

#8886 added a test which unsets `__CARGO_TEST_ROOT`, but that environment variable is there for a reason. This causes problems as it causes that test to load the `.cargo/config` from the real home directory, which if it contains a `[cargo-new]` section, causes the test to fail.

The fix here is to change `find_tests_git_config` so that it behaves more like the real git config loader, but avoids escaping the test sandbox.  There are some subtle issues here, like #7469, which I believe should still work correctly.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-testing-cargo-itself Area: cargo's tests C-bug Category: bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants