services/horizon: Refactors integration test framework to allow custom parameter testing.

[Shaptic]

PR Checklist

PR Structure

• This PR has reasonably narrow scope (if not, break it down into smaller PRs).
• This PR avoids mixing refactoring changes with feature changes (split into two PRs
  otherwise).
• This PR's title starts with name of package that is most changed in the PR, ex.
  services/friendbot, or all or doc if the changes are broad or impact many
  packages.

Thoroughness

• This PR adds tests for the most critical parts of the new functionality or fixes.
• I've updated any docs (developer docs, .md
  files, etc... affected by this change). Take a look in the docs folder for a given service,
  like this one.

Release planning

• I've updated the relevant CHANGELOG (here for Horizon) if
  needed with deprecations, added features, breaking changes, and DB schema changes.
• I've decided if this PR requires a new major/minor version according to
  semver, or if it's mainly a patch change. The PR is targeted at the next
  release branch if it's not a patch change.

What

This introduces the IntegrationTest.Config.HorizonParameters field, which allows you to customize the parameters passed to Horizon, and refactors the tests to use these accordingly. It also introduces FatalTestSuite, which is a way to run integration tests and see if they should fail.

Note: This is mostly intended a design proposal and an opportunity to get feedback before I dive into actual testing of the combinatorial explosion of parameters we have. There are a few tests and validators included to demonstrate how said tests would look.

Why

In order to facilitate configuration testing (see #3193), we need to be able to run Horizon with arbitrary combinations of parameters.

Known limitations

[tamirms]

I don't understand what's happening here. Could you explain what suite.Exits is doing and why it's necessary to implement the control flow in this way?

[paulbellamy]

As I understand it, (correct me if I'm wrong @Shaptic) we need this because our config is injected via global variables, and ad-hoc places in the code fetching environment variables. So to run the app with different configs we have to launch a whole new subprocess...

If we had a more controlled way of passing config into horizon we could do something like:

import "github.com/services/horizon/cmd"

c := cmd.RootCmd
c.SetEnv(map[string]string{"ENV_VARS_HERE": "1"}) // Not sure how to do this with cobra
c.SetArgs([]string{"db", "migrate", "up"})
assert.NoError(t, c.Execute()) // or assert.Error(t, c.Execute(), "whatever we expect")

But yeah the control flow around the subprocess thing in Exits is quite weird either way.

[Shaptic]

You're almost there, @paulbellamy.

The main problem is that Horizon dies via log.Fatal if the configuration options are invalid. Since the purpose of this framework is exactly to test whether or not Horizon treats invalid combinations as invalid (and that it treats valid combinations like it should), we basically need a way to say "did Horizon die as expected?" Since log.Fatal kills the process, the test also immediately dies (we do use c.Execute() in the integration tests [refer to Test.StartHorizon()]) at the corresponding cmd.Execute() call.

Thus, we need this (hacky, but functional) suite.Exits() approach to detect Horizon exiting expectedly by launching the entire test as a subprocess.

[paulbellamy]

Yeah, I'm saying that, in an ideal world, only place in the codebase that should do log.Fatal, or os.Exit is func main()

[Shaptic]

Sure, agreed! But that's a bigger refactor than is in scope here, unfortunately.

The alternative is to run the entirety of Horizon as a subprocess rather than "natively" in a go func() {}; that refactor may actually be "better" in a way because it enables us to test subcommands, too (e.g. migrate).

[paulbellamy]

Nit, should this bit about log.Fatal and it dying be a comment on the Exits method?

[paulbellamy]

As I understand it, (correct me if I'm wrong @Shaptic) we need this because our config is injected via global variables, and ad-hoc places in the code fetching environment variables. So to run the app with different configs we have to launch a whole new subprocess...

If we had a more controlled way of passing config into horizon we could do something like:

import "github.com/services/horizon/cmd"

c := cmd.RootCmd
c.SetEnv(map[string]string{"ENV_VARS_HERE": "1"}) // Not sure how to do this with cobra
c.SetArgs([]string{"db", "migrate", "up"})
assert.NoError(t, c.Execute()) // or assert.Error(t, c.Execute(), "whatever we expect")

But yeah the control flow around the subprocess thing in Exits is quite weird either way.

[paulbellamy]

I'd do these as two separate tests, and extract the setup (which is the common part) instead of extracting the test (which is the unique part).

// Should this be a tempdir?
const storagePath string = "./test_captive-core-works"

var defaultCaptiveCoreConfig = map[string]string{
  "captive-core-storage-path":       storagePath,
  "captive-core-reuse-storage-path": "true",
  horizon.StellarCoreBinaryPathName: os.Getenv("CAPTIVE_CORE_BIN"),
  horizon.CaptiveCoreConfigPathName: "./captive-core.toml",
  horizon.StellarCoreURLFlagName:    "",
  horizon.StellarCoreDBURLFlagName:  "",
}

func TestCaptiveCoreCreatesExpectedDirsOnLaunch(t *testing.T) {
  // ensure that running Captive Core creates a
  // sensible directory structure.
  //
  // Pass "rootDirectory" set to whatever it is you pass to
  // "--captive-core-storage-path".
    
  if _, ok := os.LookupEnv("HORIZON_INTEGRATION_ENABLE_CAPTIVE_CORE"); !ok {
    t.Skip() // explained above
  }
  
  cleanup := launchHorizon(t, defaultConfig)
  defer cleanup()

  storageDir := path.Join(rootDir, "captive-core")
  coreConf := path.Join(storageDir, "stellar-core.conf")
  
  assert.DirExists(t, rootDir)
  assert.DirExists(t, storageDir)
  assert.FileExists(t, coreConf)
}

func TestCaptiveCoreConfigExcludesBucketDirPathEntry(t *testing.T) {
  // ensure the Captive Core auto-generated configuration
  // file doesn't contain the BUCKET_DIR_PATH entry, which is forbidden when using
  // Captive Core.
  //
  // Pass "rootDirectory" set to whatever it is you pass to
  // "--captive-core-storage-path".
  
  if _, ok := os.LookupEnv("HORIZON_INTEGRATION_ENABLE_CAPTIVE_CORE"); !ok {
    t.Skip() // explained above
  }
  
  cleanup := launchHorizon(t, defaultConfig)
  defer cleanup()
  
  coreConf := path.Join(rootDir, "captive-core", "stellar-core.conf")
  assert.FileExists(t, coreConf)
  
  result, err := ioutil.ReadFile(coreConf)
  assert.NoError(t, err)
  
  isBucketPathSet := strings.Contains(string(result), "BUCKET_DIR_PATH")
  assert.False(t, isBucketPathSet)
}

// Paul: Or whatever you need to do to launch horizon here.
func launchHorizon(t *testing.T, config map[string]string) (func()) {
  // Paul: Should this be a temp file? Actually, why does this need to be a file?
  cleanupConfig := createCaptiveCoreConfig("./captive-core.toml", CAPTIVE_CORE_CONFIG_STATE_TOML)
  
  test := NewParameterTest(t, config)
  defer os.RemoveAll(storagePath)
  
  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)
  
  // Paul: Do we need to kill horizon when the test finishes?
  return cleanupConfig
}

[Shaptic]

I think it looks this way because there aren't enough example tests yet, but the intent of the framework here is essentially to associate certain "validation functions" with each parameter. You grouped the parameters together into defaultCaptiveCoreConfig, but in fact this is supposed to be the unique part. Otherwise, we aren't actually testing the parameter combinations!

However, I do think there's merit to having this default, and then instead modifying only what we need within the test. I'll tinker with it and push my ideas.

[paulbellamy]

Got it, ok. In that case, maybe we should use t.Run("some name", ...) to label them instead of the matrix function?

Basically, this bit:

runParameterMatrix(test, []ValidatorFunc{
  func() { validateCaptiveCoreDiskState(test, STORAGE_PATH) },
  func() { validateNoBucketDirPath(test, STORAGE_PATH) },
})

Obscures that these are the important parts, and are actually two separate tests.

[Shaptic]

Hm, seems reasonable. Done in 2d98e42, specifically here.

[paulbellamy]

Cool. Looks better at least. As a nit, I'd probably inline validateNoBucketDirPath, and the other one (since they're only used in one place). But it's good enough for now.

[Shaptic]

Fair, but again the idea is that these would be reused in new tests that also wanted to confirm the state of e.g. --captive-core-storage-path.

[paulbellamy]

Definitely more refactoring we could do to make testing this simpler, but much better than we had, and more than good enough for now. 💯