do you think it's worth taking the time to build some sort of default config to be used in our testing environment instead of being vulnerable to the underlying system's settings?

I gather you're thinking of something like this?

I worry that using a pre-canned config with tests would prevent us from catching issues with config set at the system level (set by the Git distribution, and can differ by OS) or global level by users that Desktop should handle.

I think in this case what I should have done was ensure the local config for a repository is set correctly for what is being tested (which overrides whatever may be set globally). That's the approach I continue to recommend, particularly with complex areas with plenty of config options like git pull.

These tests were there to help me wrap my head around the current pull behaviour based on expected config values as I dove into "pull with rebase" and seeing them fail now was a good chance to revisit them and question what value they actually had to the feature. I worry that abstracting away the global config away will not give us a chance to identify and re-evaluate our tests as things evolve.

We also don't have an easy way to inject the config for each test (getGlobalConfigValue was important to get under test otherwise we'd be overwriting the user's config), and I worry this might add complexity to the overall Git API surface.

@shiftkey I'm not sure how the code snippet above relates to the question?

I understand your concerns but I think ideally we would enumerate our conditions and assumptions in our test harnesses instead of relying on variations in underlying system variance to catch bugs. (This can lead to difficult to reproduce test failures that don't provide much data to us and can be frustrating for our collaborators.)

For this PR, I'd prefer a fixture-ish approach that uses helper functions for setting up certain config conditions. Perhaps returning an entire config is a bit too much to take on now, but it would be nice to start collecting our different configs and modifiers for our testing into a single place.

Perhaps something like a setPullRebaseConfig(repositoryPath, boolean) or even PullRebaseButNotFF(repositoryPath) in a helper file?

I'm not sure how the code snippet above relates to the question?

This is the ceremony I had in mind when you were asking about a "testing environment instead of being vulnerable to the underlying system's settings" - creating a new global config on disk, assigning this to the HOME environment variable, and then passing that through to the Git command to represent the "global" configuration (each API under test would need to support this).

That sounds different to this:

For this PR, I'd prefer a fixture-ish approach that uses helper functions for setting up certain config conditions.

This is interesting to me from the perspective of making setup more clear:

diff --git a/app/test/helpers/local-config.ts b/app/test/helpers/local-config.ts
new file mode 100644
index 000000000..0283d80f9
--- /dev/null
+++ b/app/test/helpers/local-config.ts
@@ -0,0 +1,11 @@
+import { Repository } from '../../src/models/repository'
+import { GitProcess } from 'dugite'
+
+export async function setupLocalConfig(
+  repository: Repository,
+  localConfig: Iterable<[string, string]>
+) {
+  for (const [key, value] of localConfig) {
+    await GitProcess.exec(['config', key, value], repository.path)
+  }
+}
diff --git a/app/test/unit/git/pull/ahead-and-behind-test.ts b/app/test/unit/git/pull/ahead-and-behind-test.ts
index 6ecff4bb4..356e61c8a 100644
--- a/app/test/unit/git/pull/ahead-and-behind-test.ts
+++ b/app/test/unit/git/pull/ahead-and-behind-test.ts
@@ -13,6 +13,7 @@ import {
 } from '../../../helpers/repository-scaffolding'
 import { getTipOrError, getRefOrError } from '../../../helpers/tip'
 import { GitProcess } from 'dugite'
+import { setupLocalConfig } from '../../../helpers/local-config'
 
 const featureBranch = 'this-is-a-feature'
 const origin = 'origin'
@@ -59,14 +60,10 @@ describe('git/pull', () => {
       let newTip: Commit
 
       beforeEach(async () => {
-        await GitProcess.exec(
-          ['config', '--local', 'pull.rebase', 'false'],
-          repository.path
-        )
-        await GitProcess.exec(
-          ['config', '--local', 'pull.ff', 'false'],
-          repository.path
-        )
+        await setupLocalConfig(repository, [
+          ['pull.rebase', 'false'],
+          ['pull.ff', 'false'],
+        ])
 
         previousTip = await getTipOrError(repository)

It doesn't quite address the presence of the global config problem - we'd still need to pass in the new HOME environment variable for each Git operation under test.

Would an API like ☝️ help unblock this PR?

This is the ceremony I had in mind

@shiftkey ah! thanks for clarifying. yeah i'm thinking we can override global, system-level configs with local configs in the repositories we use in tests.

Would an API like ☝️ help unblock this PR?

👍 👍 👍 yes! this looks great.

@outofambit ok cool, I'll add that work into this PR and see how it changes things. There's some places where we change the config on-the-fly that I'll call out as being different to this sort of "setup" case we're discussing here.

Since this is tech-debt and @outofambit was involved in guiding the direction of this PR, I'm going to let it hang around until she gets a chance to take a look.