services/horizon: Move integration tests away from stellar/quickstart. #3143

Shaptic · 2020-10-20T00:34:43Z

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 moves the integration test infrastructure away from creating a stellar/quickstart:testing instance for each test, preferring instead to use the modular Docker Compose method.

Why

We want more flexibility with our integration tests; see #3037 for more discussion.

This closes #3031 and closes #3121.

Known limitations

A couple of kinks:

Ctrl+C is unreliable as far as cleanup goes, but per discussion below we may want to address this in a later PR, since it it that failing tests are properly handled on the next run.

go.list

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

bartekn · 2020-10-22T14:15:44Z

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

@@ -635,3 +518,9 @@ func panicIf(err error) {
 		panic(err)
 	}
 }
+
+func fatalIf(t *testing.T, err error) {


Not in a scope of this PR but can we replace panicIf with fatalIf. I can be wrong but I think I noticed that Cleanup is not called in case of panic.

bartekn · 2020-10-22T14:20:16Z

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

+	// directory of the project.
+	current, err := os.Getwd()
+	fatalIf(t, err)
+	for !directoryContains(current, "go.mod") {


I think in correctly configured environment you should be able to find the project root by concatenating $GOPATH env variable and src/github.com/stellar/go.

From what I gather, this would give you the go get version on the master branch, but I think we're much likelier to want to use our local branch/repo. Unless, of course, I don't fall into the category of a "correctly configured environment" 😅

This is what I mean by correctly configured environment but I should rather say typical environment. AFAIR, since go modules were introduced there were some issues when the code was outside $GOPATH. We can leave existing code if it works, it was more nit.

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

bartekn · 2020-10-22T14:26:17Z

services/horizon/docker/docker-compose.standalone.yml

  core-upgrade:
    restart: on-failure
    image: curlimages/curl:7.69.1
-    command: ["-v", "-f", "http://host.docker.internal:11626/upgrades?mode=set&upgradetime=1970-01-01T00:00:00Z&protocolversion=13"]
+    command: ["-v", "-f", "http://host.docker.internal:11626/upgrades?mode=set&upgradetime=1970-01-01T00:00:00Z&protocolversion=${PROTOCOL_VERSION:-14}"]


Maybe we can upgrade to the latest version available if it's not set?

2opremio · 2020-10-22T17:20:52Z

@Shaptic could you please make sure that the captive-core integration tests (which are currently disabled until #3144 is finished ) are also ported?

2opremio · 2020-10-30T12:35:21Z

.circleci/config.yml

@@ -419,6 +418,9 @@ jobs:
          command: |
            echo "export HORIZON_INTEGRATION_TESTS=true" >> $BASH_ENV
            echo "export HORIZON_BIN_DIR=~/go/src/github.com/stellar/go" >> $BASH_ENV
+      - run:
+          name: Pull latest Stellar Core image
+          command: docker pull stellar/stellar-core


I think I would pin this to a specific version

It's worth checking the latest version to confirm it tests work with the latest core version.

services/horizon/docker/docker-compose.integration-tests.yml

2opremio · 2020-10-30T12:38:36Z

services/horizon/docker/stellar-core-integration-tests.cfg

@@ -0,0 +1,28 @@
+# simple configuration for a standalone test "network"
+# see stellar-core_example.cfg for a description of the configuration parameters


Did you consider using a template instead of having multiple configuration files?

could you elaborate on how we could use a template here (e.g. what templating engine would we use, how would we invoke the template to generate the actual configuration we need) ?

e.g. https://golang.org/pkg/text/template/ would probably be enough , you would need to use Go (e.g. from the integration tests) to fill it in.

ok, I think we'll definitely take this approach if we need to generate several stellar core configurations for the different integration test cases. for now it seems that this stellar core configuration will work for all the integration test cases

2opremio · 2020-10-30T12:40:37Z

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

-			context.Background(),
-			types.ContainerListOptions{All: true, Quiet: true})
+	// Lets you check if a particular directory contains a file.
+	directoryContains := func(root string, needle string) bool {


Suggested change

directoryContains := func(root string, needle string) bool {

directoryContainsFileName := func(dir string, filenameToFind string) bool {

2opremio · 2020-10-30T12:43:26Z

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

-	}
+	// Walk up the tree until we find "go.mod", which we treat as the root
+	// directory of the project.
+	current, err := os.Getwd()


Doesn't this assume you run the integration tests from the current directory? I am not sure this will always be the case (e.g. from IDEs).

Maybe this will work https://stackoverflow.com/a/18537792/1914440

The only assumption is that the test is run from somewhere within the project. I tried a few methods: your linked one, an approach via runtime.Caller, and the above. This one appeared to be the most reliable; however, I could be wrong (esp. since Goland, Sublime, and VS Code probably all do things slightly differently) and we may need to adjust this.

2opremio · 2020-10-30T12:51:52Z

I've only tested on Linux so far (for obvious reasons) which has some quirks relative to other platforms, purely judging from the start.sh script we have.

Ctrl+C reaaaally does not work well, and I haven't looked at what happens when tests fail, either.

It's still TBD whether or not we want to run the horizon-postgres container: from a "just let me run the tests" perspective, it might be better/cleaner to keep the DBs as self-contained as possible.

Is this resolved?

bartekn · 2020-10-30T13:28:00Z

services/horizon/internal/integration/protocol14_state_verifier_test.go

 	assert.NoError(t, err)

 	// Wait for the third checkpoint ledger and state verification trigger
 	// Core will push to history archives *after* checkpoint ledger
-	itest.CloseCoreLedgersUntilSequence(thirdCheckpoint + 1)
+	err = itest.CloseCoreLedgersUntilSequence(thirdCheckpoint + 1)


tamirms · 2020-10-30T13:43:44Z

I've only tested on Linux so far (for obvious reasons) which has some quirks relative to other platforms, purely judging from the start.sh script we have.

I've removed the platform specific quirks so it should work on all operating systems which can run docker-compose. I've run the tests locally on my macbook and on linux via circleci.

It's still TBD whether or not we want to run the horizon-postgres container: from a "just let me run the tests" perspective, it might be better/cleaner to keep the DBs as self-contained as possible.

Let's not run the horizon-postgres container for now.

Ctrl+C reaaaally does not work well, and I haven't looked at what happens when tests fail, either.

When the tests fail, the cleanup hook will destroy all the docker containers created by the integration tests.
Ctrl+C seems to abort instantly for me. However, when I Ctrl+C the docker containers from the integration tests are still running.

Stellar Core recently modified its database schema which consequently broke the SQL queries used in dump_core_db.sh. This commit fixes the SQL queries so that it works with the latest version of Stellar Core. This commit also adds support for claimable balances which were introduced in Protocol 14/15.

Shaptic · 2020-11-03T07:53:32Z

I refactored a small bit regarding how we find the docker-compose file in the latest changes 0c0d0b7.

Unrelated to the above (fails w/ and w/o the changes), but on my local it looks like a single test will fail intermittently with a "still ingesting" error. I've tried but can't track down the cause; it appears to only happen on either TestProtocol14StateVerifier or TestProtocol15Basics. Not when run individually, mind you, but if all of the tests are run.

I can definitely see our introduction of concurrency to testing (incl. the captive core stuff) causing hard-to-track-down and Heisenbug-esque test failures. I'm not sure if this is just something mucked up with my environment, but are we ready to merge this?

tamirms · 2020-11-03T13:52:37Z

@Shaptic I thought I fixed the "still ingesting" error earlier in 016c91e . But then I saw your comment and was able to reproduce the error in CircleCI. I finally figured out the cause and it's because the root endpoint determines the sequence numbers from a global ledger.State variable. This global variable is mutable and persists between test runs. Horizon periodically calls ledger.SetState() to update the variable. However, it's possible that the check in 016c91e is executed before ledger.SetState() is called for the first time by the new Horizon instance.

In 027a48c , I make sure to clear the ledger state after every test run. This will ensure that waitForHorizon() will make sure to wait until Horizon ingestion is ready.

I have run the integration tests on CircleCI 5 times with this latest fix and so far there haven't been any failures.

Shaptic · 2020-11-03T17:28:34Z

Wow, ridiculously subtle, thanks for tracking that down. I ran the suite a couple times locally, as well: only seeing green ✔️ 🎉. I think we can push back resolving the Ctrl+C inconsistencies to another PR to get this in and running, what do you think?

@stellar/horizon-committers anything we're missing? I think we're ready to merge.

services/horizon/docker/docker-compose.standalone.yml

bartekn · 2020-11-05T14:34:23Z

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

+			i.app.Close()
+			// Clear the ledger state otherwise the root response
+			// will contain ledger information from the previous test run
+			ledger.SetState(ledger.State{})


Good catch @tamirms. Created an issue for this: #3195.

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

Co-authored-by: Bartek Nowotarski <[email protected]>

horizon/ledger.State is a global variable. This is a bad pattern and caused some issues (see #3143). We should make the state local to horizon.App and pass a pointer to it to other parts of the system that need it.

Shaptic added 5 commits October 19, 2020 17:18

Use Protocol 14 in standalone core setup

b8bda97

Attempt to get tests to use Docker Compose over stellar/quickstart

2ba097b

Fix shadowed variable (thx go vet)

ede737e

Update Go modules to latest versions

068b06a

Add some small cleanups & comments

11800e6

Shaptic self-assigned this Oct 20, 2020

cla-bot bot added the cla: yes label Oct 20, 2020

Shaptic changed the title ~~services/horizon: Move away from the stellar/quickstart image in integration tests.~~ services/horizon: Move integration tests away from stellar/quickstart. Oct 20, 2020

Shaptic added 5 commits October 19, 2020 17:45

Avoid directory traversal entirely, preferring absolute paths

104de46

Allow protocol version to be configurable

0cd8f9a

Should fix the CI name resolution error & improves env setup

c0399b6

Reduce verbosity

843c346

Make version check more generic

d38381d

Shaptic requested a review from a team October 20, 2020 20:15

Shaptic marked this pull request as ready for review October 20, 2020 20:15

Merge branch 'master' into no-more-quickstart

86b0453

bartekn reviewed Oct 22, 2020

View reviewed changes

Shaptic and others added 12 commits October 22, 2020 11:40

Drop unnecessary method

246e625

Clean up errors, path handling, and hostname resolution

22197a8

Drop more unneeded docker stuff

52a5a23

Undo the changes to module dependencies

0833a95

Okay so not *every*thing docker related is useless

71b9a0a

Drop docker dependencies entirely

6c2e103

Merge branch 'master' into no-more-quickstart

7d59244

wow this is complicated

22bd660

Fix integration test commands

87a6a03

Merge branch 'master' into no-more-quickstart

94cc6b9

Handle captive-core runs a little more graciously

9d90135

Merge branch 'master' into no-more-quickstart

dcbd30e

2opremio reviewed Oct 30, 2020

View reviewed changes

services/horizon/docker/docker-compose.integration-tests.yml Show resolved Hide resolved

2opremio reviewed Oct 30, 2020

View reviewed changes

bartekn reviewed Oct 30, 2020

View reviewed changes

Shaptic added 2 commits November 2, 2020 23:54

Update comments, enhance compose file lookup, and move it to a helper

0c0d0b7

Merge branch 'master' into no-more-quickstart

a5096f4

Shaptic force-pushed the no-more-quickstart branch from 4ede3af to a5096f4 Compare November 3, 2020 07:55

Shaptic and others added 4 commits November 3, 2020 00:06

Appease go vet like the Allies did

1223613

Add logs to debug still ingesting error

34c5830

Clear ledgerstate between test runs

027a48c

Use protocol 15 in docker-compose standalone config

44370a3

tamirms added 2 commits November 4, 2020 14:00

Merge branch 'master' into no-more-quickstart

854d694

Merge branch 'master' into no-more-quickstart

c6623f0

bartekn mentioned this pull request Nov 5, 2020

services/horizon/ledger.State package in Horizon is a global variable #3195

Closed

bartekn approved these changes Nov 5, 2020

View reviewed changes

tamirms and others added 3 commits November 5, 2020 16:34

Update services/horizon/docker/docker-compose.standalone.yml

7332b14

Co-authored-by: Bartek Nowotarski <[email protected]>

Set ports to integers not strings

1d00fe3

Merge branch 'master' into no-more-quickstart

8deece7

Shaptic merged commit 38aec89 into stellar:master Nov 5, 2020

Shaptic deleted the no-more-quickstart branch November 5, 2020 22:20

tamirms mentioned this pull request Nov 12, 2020

services/horizon: Remove global ledger state #3216

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

services/horizon: Move integration tests away from stellar/quickstart. #3143

services/horizon: Move integration tests away from stellar/quickstart. #3143

Shaptic commented Oct 20, 2020 •

edited

Loading

bartekn Oct 22, 2020

bartekn Oct 22, 2020

Shaptic Oct 22, 2020

bartekn Oct 30, 2020

bartekn Oct 22, 2020

2opremio commented Oct 22, 2020 •

edited

Loading

2opremio Oct 30, 2020

bartekn Oct 30, 2020

2opremio Oct 30, 2020

tamirms Oct 30, 2020

2opremio Oct 30, 2020 •

edited

Loading

tamirms Oct 30, 2020

2opremio Oct 30, 2020

2opremio Oct 30, 2020

Shaptic Oct 31, 2020

2opremio commented Oct 30, 2020

bartekn Oct 30, 2020

2opremio Oct 30, 2020

tamirms commented Oct 30, 2020

Shaptic commented Nov 3, 2020 •

edited

Loading

tamirms commented Nov 3, 2020

Shaptic commented Nov 3, 2020 •

edited

Loading

bartekn Nov 5, 2020

		@@ -0,0 +1,28 @@
		# simple configuration for a standalone test "network"
		# see stellar-core_example.cfg for a description of the configuration parameters

	directoryContains := func(root string, needle string) bool {
	directoryContainsFileName := func(dir string, filenameToFind string) bool {

services/horizon: Move integration tests away from stellar/quickstart. #3143

services/horizon: Move integration tests away from stellar/quickstart. #3143

Conversation

Shaptic commented Oct 20, 2020 • edited Loading

PR Structure

Thoroughness

Release planning

What

Why

Known limitations

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

2opremio commented Oct 22, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

2opremio Oct 30, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

2opremio commented Oct 30, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tamirms commented Oct 30, 2020

Shaptic commented Nov 3, 2020 • edited Loading

tamirms commented Nov 3, 2020

Shaptic commented Nov 3, 2020 • edited Loading

Choose a reason for hiding this comment

Shaptic commented Oct 20, 2020 •

edited

Loading

2opremio commented Oct 22, 2020 •

edited

Loading

2opremio Oct 30, 2020 •

edited

Loading

Shaptic commented Nov 3, 2020 •

edited

Loading

Shaptic commented Nov 3, 2020 •

edited

Loading