Incorporate the rest of the review feedback:

 - renames, global check
 - wait for horizon properly
 - drop extra check
 - test env-var restoration

services/horizon/internal/integration/parameters_test.go

@@ -8,7 +8,6 @@ import (
 	"path"
 	"strings"
 	"testing"
-	"time"
 
 	horizon "github.com/stellar/go/services/horizon/internal"
 	"github.com/stellar/go/services/horizon/internal/test/integration"
@@ -17,12 +16,12 @@ import (
 	"github.com/stretchr/testify/suite"
 )
 
-func NewParameterTest(t *testing.T, params map[string]string) *integration.Test {
+func NewParameterTest(t *testing.T, params, envvars map[string]string) *integration.Test {
 	config := integration.Config{
 		ProtocolVersion:    17,
-		DontStartHorizon:   true,
+		SkipHorizonStart:   true,
 		HorizonParameters:  params,
-		HorizonEnvironment: map[string]string{}, // TODO: test these too
+		HorizonEnvironment: envvars,
 	}
 	return integration.NewTest(t, config)
 }
@@ -69,50 +68,80 @@ func (suite *FatalTestCase) TestBucketDirDisallowed() {
 	// In CI, we run our integration tests twice: once with Captive Core
 	// enabled, and once without. *These* tests only run with Captive Core
 	// configured properly (specifically, w/ the CAPTIVE_CORE_BIN envvar set).
-	if _, ok := os.LookupEnv("HORIZON_INTEGRATION_ENABLE_CAPTIVE_CORE"); !ok {
+	if !integration.RunWithCaptiveCore {
 		suite.T().Skip()
 	}
 
-	defer createCaptiveCoreConfig(
-		"./captive-core.toml", BUCKET_DIR_DISALLOWED_TOML)()
+	confName, cleanup := createCaptiveCoreConfig(BUCKET_DIR_DISALLOWED_TOML)
+	defer cleanup()
 
 	const STORAGE_PATH string = "./test_no-bucket-dir"
 	test := NewParameterTest(suite.T(), map[string]string{
 		"captive-core-storage-path":       STORAGE_PATH,
-		horizon.CaptiveCoreConfigPathName: "./captive-core.toml",
+		horizon.CaptiveCoreConfigPathName: confName,
 		horizon.StellarCoreBinaryPathName: os.Getenv("CAPTIVE_CORE_BIN"),
-	})
+	}, map[string]string{})
 	defer os.RemoveAll(STORAGE_PATH)
 
 	suite.Exits(func() { test.StartHorizon() })
 }
 
+func (suite *FatalTestCase) TestEnvironmentPreserved() {
+	// Who tests the tests? This test.
+	//
+	// It ensures that the global OS environmental variables are preserved after
+	// running an integration test.
+	t := suite.T()
+
+	// Note that we ALSO need to make sure we don't modify parent env state.
+	if value, isSet := os.LookupEnv("STELLAR_CORE_BINARY_PATH"); isSet {
+		defer func() {
+			os.Setenv("STELLAR_CORE_BINARY_PATH", value)
+		}()
+	}
+
+	err := os.Setenv("STELLAR_CORE_BINARY_PATH", "dummy value")
+	assert.NoError(t, err)
+
+	test := NewParameterTest(t,
+		map[string]string{},
+		map[string]string{
+			// intentionally invalid parameter combination
+			"CAPTIVE_CORE_CONFIG_PATH": "",
+			"STELLAR_CORE_BINARY_PATH": "/nonsense",
+		},
+	)
+
+	suite.Exits(func() { test.StartHorizon() })
+	test.Shutdown()
+
+	envValue := os.Getenv("STELLAR_CORE_BINARY_PATH")
+	assert.Equal(t, "dummy value", envValue)
+}
+
 // Ensures that the filesystem ends up in the correct state with Captive Core.
 func TestCaptiveCoreConfigFilesystemState(t *testing.T) {
-	if _, ok := os.LookupEnv("HORIZON_INTEGRATION_ENABLE_CAPTIVE_CORE"); !ok {
+	if !integration.RunWithCaptiveCore {
 		t.Skip() // explained above
 	}
 
-	defer createCaptiveCoreConfig(
-		"./captive-core.toml", CAPTIVE_CORE_CONFIG_STATE_TOML)()
+	confName, cleanup := createCaptiveCoreConfig(CAPTIVE_CORE_CONFIG_STATE_TOML)
+	defer cleanup()
 
 	const STORAGE_PATH string = "./test_captive-core-works"
 	test := NewParameterTest(t, map[string]string{
 		"captive-core-storage-path":       STORAGE_PATH,
 		"captive-core-reuse-storage-path": "true",
 		horizon.StellarCoreBinaryPathName: os.Getenv("CAPTIVE_CORE_BIN"),
-		horizon.CaptiveCoreConfigPathName: "./captive-core.toml",
+		horizon.CaptiveCoreConfigPathName: confName,
 		horizon.StellarCoreURLFlagName:    "",
 		horizon.StellarCoreDBURLFlagName:  "",
-	})
+	}, map[string]string{})
 	defer os.RemoveAll(STORAGE_PATH)
 
 	err := test.StartHorizon()
 	assert.NoError(t, err)
-	// FIXME: IntegrationTest needs a big refactor so we can properly wait for
-	//        Captive Core. This needs a lot more thought so this workaround is
-	//        an unfortunate temporary necessity...
-	time.Sleep(10 * time.Second)
+	test.WaitForHorizon()
 
 	runParameterMatrix(test, []ValidatorFunc{
 		func() { validateCaptiveCoreDiskState(test, STORAGE_PATH) },

services/horizon/internal/test/integration/integration.go

@@ -39,6 +39,10 @@ const (
 	historyArchivePort          = 1570
 )
 
+var (
+	RunWithCaptiveCore = os.Getenv("HORIZON_INTEGRATION_ENABLE_CAPTIVE_CORE") != ""
+)
+
 type Config struct {
 	PostgresURL           string
 	ProtocolVersion       int32
@@ -47,7 +51,7 @@ type Config struct {
 
 	// Weird naming here because bools default to false, but we want to start
 	// Horizon by default.
-	DontStartHorizon bool
+	SkipHorizonStart bool
 
 	// If you want to override the default parameters passed to Horizon, you can
 	// set this map accordingly. All of them are passed along as --k=v, but if
@@ -118,7 +122,7 @@ func NewTest(t *testing.T, config Config) *Test {
 
 	// We either test Captive Core through environment variables, or through
 	// custom Horizon parameters.
-	if os.Getenv("HORIZON_INTEGRATION_ENABLE_CAPTIVE_CORE") != "" {
+	if RunWithCaptiveCore {
 		i.coreConfig.binaryPath = os.Getenv("CAPTIVE_CORE_BIN")
 		i.coreConfig.configPath = filepath.Join(composePath, "captive-core-integration-tests.cfg")
 	}
@@ -130,25 +134,18 @@ func NewTest(t *testing.T, config Config) *Test {
 		i.coreConfig.configPath = value
 	}
 
-	if !bothOrNeitherString(i.coreConfig.binaryPath, i.coreConfig.configPath) {
-		t.Fatalf(`Both the Stellar Core binary and Captive Core configuration
-needs to be set (or neither). Try setting CAPTIVE_CORE_BIN in your environment
-or filling in Config.HorizonParameters/HorizonEnvironment with the appropriate
-parameter(s): binPath=%s, configPath=%s`, i.coreConfig.binaryPath, i.coreConfig.configPath)
-	}
-
 	// Only run Stellar Core container and its dependencies.
 	i.runComposeCommand("up", "--detach", "--quiet-pull", "--no-color", "core")
 	i.cclient = &stellarcore.Client{URL: "http://localhost:" + strconv.Itoa(stellarCorePort)}
 	i.waitForCore()
 	i.prepareShutdownHandlers()
 
-	if !config.DontStartHorizon {
+	if !config.SkipHorizonStart {
 		if innerErr := i.StartHorizon(); innerErr != nil {
 			t.Fatalf("Failed to start Horizon: %v", innerErr)
 		}
 
-		i.waitForHorizon()
+		i.WaitForHorizon()
 	}
 
 	return i
@@ -211,7 +208,7 @@ func (i *Test) RestartHorizon() error {
 		return err
 	}
 
-	i.waitForHorizon()
+	i.WaitForHorizon()
 	return nil
 }
 
@@ -264,7 +261,7 @@ func (i *Test) StartHorizon() error {
 	coreBinaryPath := i.coreConfig.binaryPath
 	captiveCoreConfigPath := i.coreConfig.configPath
 
-	var DEFAULT_ARGS = map[string]string{
+	defaultArgs := map[string]string{
 		"stellar-core-url": i.cclient.URL,
 		"stellar-core-db-url": fmt.Sprintf(
 			"postgres://postgres:%s@%s:%d/stellar?sslmode=disable",
@@ -376,7 +373,7 @@ func (i *Test) waitForCore() {
 	i.t.Fatal("Core could not sync after several attempts")
 }
 
-func (i *Test) waitForHorizon() {
+func (i *Test) WaitForHorizon() {
 	for t := 200; t >= 0; t -= 1 {
 		i.t.Log("Waiting for ingestion and protocol upgrade...")
 		root, err := i.hclient.Root()