Skip to content

Unset GIT_ASKPASS in tests that assume it is unset #1505

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

Merged
merged 1 commit into from
Aug 10, 2024

Conversation

EliahKagan
Copy link
Member

Two of the askpass tests assume GIT_ASKPASS is not set. Before this change, they wrongly fail when it is set, even if the code under test is working correctly.

  1. ssh_askpass_is_used_as_fallback
  2. ssh_askpass_does_not_override_current_value

As other tests that deliberately involve GIT_ASKPASS (and therefore do not need to be modified) attest, when it is set:

  1. SSH_ASKPASS is not used as a fallback (GIT_ASKPASS deliberately takes precedence).

  2. Although SSH_ASKPASS still does not override the current value, GIT_ASKPASS does override it, so checking for equality to the current value fails.

Since those test cases are among those that already have the serial attribute and already temporarily modify the environment, this extends that modification by having it also temporarily unset GIT_ASKPASS if it was set.

An alternative fix could be to pass false as the first argument to apply_environment, so that GIT_ASKPASS is not considered. Which of these approaches is better may depend on exactly what behavior we are trying to test. (Maybe even two more test cases should be added, to test it both ways.)


Situations where GIT_ASKPASS is set include dev containers. I discovered this bug while running tests in the dev container environment added in #1502, where all tests passed except the two mentioned above:

failures:

---- options::apply_environment::ssh_askpass_does_not_override_current_value stdout ----
thread 'options::apply_environment::ssh_askpass_does_not_override_current_value' panicked at gix-prompt/tests/options/mod.rs:68:9:
assertion `left == right` failed
  left: "/vscode/bin/linux-x64/eaa41d57266683296de7d118f574d0c2652e1fc4/extensions/git/dist/askpass.sh"
 right: "current"
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

---- options::apply_environment::ssh_askpass_is_used_as_fallback stdout ----
thread 'options::apply_environment::ssh_askpass_is_used_as_fallback' panicked at gix-prompt/tests/options/mod.rs:50:9:
assertion `left == right` failed
  left: "/vscode/bin/linux-x64/eaa41d57266683296de7d118f574d0c2652e1fc4/extensions/git/dist/askpass.sh"
 right: "fallback"


failures:
    options::apply_environment::ssh_askpass_does_not_override_current_value
    options::apply_environment::ssh_askpass_is_used_as_fallback

test result: FAILED. 7 passed; 2 failed; 0 ignored; 0 measured; 0 filtered out; finished in 1.70s

error: test failed, to rerun pass `-p gix-prompt --test prompt`

All other tests passed already, and all tests pass with this change.

Two of the askpass tests assume `GIT_ASKPASS` is not set. Before
this change, they wrongly fail when it is set, even if the code
under test is working correctly.

1. ssh_askpass_is_used_as_fallback
2. ssh_askpass_does_not_override_current_value

As other tests that deliberately involve `GIT_ASKPASS` (and
therefore do not need to be modified) attest, when it is set:

1. `SSH_ASKPASS` is not used as a fallback (`GIT_ASKPASS`
   deliberately takes precedence).

2. Although `SSH_ASKPASS` still does not override the current
   value, `GIT_ASKPASS` does override it, so checking for equality
   to the current value fails.

Since those test cases are among those that already have the
`serial` attribute and already temporarily modify the environment,
this extends that modification by having it also temporarily unset
`GIT_ASKPASS` if it was set.
@Byron Byron merged commit 3e9d3de into GitoxideLabs:main Aug 10, 2024
19 checks passed
@Byron
Copy link
Member

Byron commented Aug 10, 2024

Thanks a lot!

(I merged in a rush, if I missed anything in the PR description please let me know)

@EliahKagan EliahKagan deleted the askpass branch August 10, 2024 07:26
@EliahKagan
Copy link
Member Author

(I merged in a rush, if I missed anything in the PR description please let me know)

You probably did not miss anything. But if you have thoughts on this then I am interested and I'd be pleased to make a follow-up PR if called for:

An alternative fix could be to pass false as the first argument to apply_environment, so that GIT_ASKPASS is not considered. Which of these approaches is better may depend on exactly what behavior we are trying to test.

(But if you saw that and didn't have a response at this time, that is also definitely fine.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants