Skip to content
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

services/horizon/internal/test/integration: Run captive core integration tests #3291

Merged
merged 5 commits into from
Dec 18, 2020

Conversation

tamirms
Copy link
Contributor

@tamirms tamirms commented Dec 18, 2020

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 commit adds a CircleCI job so that the integration tests are run twice (once with captive core enabled and the other time with captive core disabled) for every PR.

The integration tests with captive core enabled are about 1.5 minutes slower than running the tests with captive core disabled.

Known limitations

[N/A]

@cla-bot cla-bot bot added the cla: yes label Dec 18, 2020
@tamirms tamirms force-pushed the captive-core-integration-tests branch 8 times, most recently from bd6dccb to 190b8da Compare December 18, 2020 12:01
@tamirms tamirms force-pushed the captive-core-integration-tests branch from 190b8da to 75a4876 Compare December 18, 2020 12:14
@tamirms tamirms marked this pull request as ready for review December 18, 2020 12:20
@tamirms tamirms requested a review from a team December 18, 2020 12:20
# From "SACJC372QBSSKJYTV5A7LWT4NXWHTQO6GHG4QDAVC2XDPX6CNNXFZ4JK"
PUBLIC_KEY="GD5KD2KEZJIGTC63IGW6UMUSMVUVG5IHG64HUTFWCHVZH2N2IBOQN7PS"
ADDRESS="localhost"
QUALITY="MEDIUM"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need a config file for reingestion?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we need to set ARTIFICIALLY_ACCELERATE_TIME_FOR_TESTING=true to be compatible with the stellar core container which is running the stand alone network.

The quorum set has no significance since we'll be doing reingestion which relies only on the history archives. But stellar-core still requires a non empty quorum set for the configuration file to be valid so I cannot leave the section empty.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we can add another stellarCoreRunnerMode: stellarCoreRunnerModeOfflineIntegrationTests that would generate config with the value set. Not urgent and maybe not needed so just sharing an idea.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bartekn another possibility is to have stellarCoreRunner set ARTIFICIALLY_ACCELERATE_TIME_FOR_TESTING=true in the config if the CheckpointFrequency in the captive core config is equal to 8.

services/horizon/internal/test/integration/integration.go Outdated Show resolved Hide resolved
services/horizon/internal/test/integration/integration.go Outdated Show resolved Hide resolved
services/horizon/internal/test/integration/integration.go Outdated Show resolved Hide resolved
Comment on lines 127 to 140
func (i *Test) RestartHorizon() {
i.app.Close()

// wait for horizon to shut down completely
time.Sleep(time.Second)

i.startHorizon(
i.horizonConfig.CaptiveCoreBinaryPath,
i.horizonConfig.CaptiveCoreConfigAppendPath,
i.horizonConfig.DatabaseURL,
false,
)
i.waitForHorizon()
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, thanks.

"--captive-core-config-append-path",
captiveCoreConfigPath,
"--captive-core-http-port",
"0",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this is because it clashes with the http port of the other core instance, I think it's worth commenting it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we run two cores at the same time in integration tests? We shouldn't, right?

Copy link
Contributor

@2opremio 2opremio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only nitpicks.

The comment about requiring a checkpointed ledger as the end range of reingestion
(if confirmed) is unrelated to this PR but I think we should create an issue about it.

@2opremio
Copy link
Contributor

BTW, huge thanks for getting this done 🥳

I think it's a big milestone for testing captive core!

@tamirms tamirms merged commit 013bf86 into stellar:master Dec 18, 2020
@tamirms tamirms deleted the captive-core-integration-tests branch December 18, 2020 18:24
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.

3 participants