-
Notifications
You must be signed in to change notification settings - Fork 70
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
Add first integration test #220
Conversation
It deploys and invokes a contract against the RPC server
.github/workflows/rust.yml
Outdated
- run: cargo test --bins --target ${{ matrix.target }} | ||
# integration tests | ||
# TODO: we should probably manage the container in the test itself | ||
# TODO: do a proper functional check on the container startup instead of waiting 15 seconds |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@2opremio , may be able to use similar test pattern that Quickstart does for blocking on soroban rpc to be up by checking the health endpoint - https://github.com/stellar/quickstart/blob/master/test_soroban_rpc_up.go, could execute similar wait loop here as another run
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can probably get something equivalent with a little bash. This is what we use in other projects:
https://github.com/stellar/starlight/blob/919096552e44eec5ec3fd6278922396926239874/.github/workflows/sdk.yml#L59-L62
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for getting a start on this, and for using the assert_cmd/fs packages to test from the outside in 👏🏻. We already have one integration test
A few concerns and suggestions inline. A while ago we moved away from embedding .wasm files from other repos. See inline for more details on why. The dependence on stellar-core is also going to block PRs. I left some suggestions inline, but @2opremio @sreuland I'm available to help with planning this out in a synchronous discussion in Discord or in a call if that's helpful.
Also fyi all tests in the tests/
repo are integration tests – that's where integration tests go in Rust projects – so I don't think we need to rename invoke_sandbox.rs
to integration_sandbox.rs
, and we could drop the prefix on the rpc server file too.
tests/integration_rpc_server.rs
Outdated
) | ||
.args(&[ | ||
"deploy", | ||
"--wasm=tests/fixtures/soroban_hello_world_contract.wasm", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like this test is dependent on a soroban-examples
.wasm file. A while ago we removed dependencies on .wasm files in other repos because we found too often they would change and break and the approach was too brittle. Instead we've moved to generating .wasm files in the repos that require them. Take a look at the rs-soroban-env repo which has an implementation of this: https://github.com/stellar/rs-soroban-env/tree/main/soroban-test-wasms. The idea is any test vectors the CLI needs should be generated by code in the same repo as the test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree it's better to track the source code than the wasm. But ... can't the contract source code also get outdated? Plus ... doesn't building the contract complicate the testing process too?
I can wait for #219 and take it from there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I took a look, it seems simple enough. I am happy to migrate the test to that later on. Let's get this merged first if you don't mind.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
after reviewing, my first observation was, any value in doing unit tests first?
rather than full integration. i.e. mocking out the soroban-rpc responses, should still be able to exercise same code paths in cli and avoid deps on wasm and other opaque concerns as far as cli knows. In past, have seen integration tests developed in separate qa type repos, and the production/code repos, usually kept their test scope to within their context as much as possible, this is just observational note.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sreuland probably, but given the largest risk with a change to the CLI is not changing the internal behavior, but the external visible behavior, I think the leaning into tests that exercise the CLI from the outside-in will have a larger impact on us providing a stable UX.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agreed, the coverage is needed and executed as often as called for, but does that mean the external coverage and it's complexity has to be implemented in this repo? I commented about de-coupling integration test and unit test concerns from repos a bit more, if that aligns, maybe external testing can be implemented as e2e cases once in a dedicated 'qa' repo instead of variations of its complexity inside each repo. Still retain the ability to execute the tests as often as needed from 'qa' repo per different environments like GHA workflow(CI) or local per configs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The follow up to this is in #222.
tests/integration_rpc_server.rs
Outdated
|
||
#[test] | ||
fn deploy_and_invoke_contract_against_rpc_server() { | ||
// This test assumes a fresh standalone network rpc server on port 8000 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need a way to run tests without running tests that require a running a stellar-core. (See #220 (comment) for why.)
I see a couple ways we could do that:
-
Have an env variable that provides the base URL of the RPC server. If it is set, use that. If it is not set, use
Command::cargo_bin
to runsoroban serve
, and then point to it. This has some huge advantages:- We can run tests twice, once with the serve, once with the real rpc server. Two tests for the price of one.
- Random dev can run the tests without the heavy rpc server / stellar-core. We maintain agility.
- People running the rpc server locally on a different port can do so.
-
Add a cfg ignore to the tests
#[cfg_attr(not(feature = "tests_e2e_rpc_server"), ignore)]
. This has the advantages that the tests are always compiled, and when running tests a line will be outputted saying a test is being skipped. This is similar to "Skip" in Go tests. -
Add a cfg to the top of the file
#![cfg(feature = "tests_e2e_rpc_server")]
. This more convenient, but the tests won't compile at all unless the feature is enabled.
I prefer the first option because it involves us always running the tests in some form. Wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need a way to run tests without running tests that require a running a stellar-core
For this particular test, I disagree. The intention is to have an end to end integration test that checks everything works.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Have an env variable that provides the base URL of the RPC server. If it is set, use that. If it is not set, use
Command::cargo_bin
to runsoroban serve
, and then point to it.
This is a good idea but it won't work today (read on). Also, it's not about testing one thing or the other. We should test both against a real RPC Server and soroban serve
(which AFAICR unfortunately wouldn't work today since it doesn't accept contract creation transactions).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It feels like we are conflating integration and unit test concerns in a single repo , ideally should be able to trigger the same coverage over functional code from either test scope, unit tests w/mocking or integration. But integration tests can happen less often(nightly, ad-hoc, PR merge) and from a separate dedicated 'qa' repo, where they spin up all the live architecture rather than duplicating that boilerplate code in each functional component repo like cli, env.
as part of soroban manual e2e testing, had spun out an issue to think about integration tests from an external repo, initially created #375 in quickstart, but it really could be a separate integration test repo and keep out of quickstart which is meant for other purposes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sreuland I agree, but I think there's more than one layer of integration testing for us to consider so I think we should do that, plus some other things locally in this repo. For example:
-
Integration testing all the way through to the soroban-rpc server, that includes a stellar-core captive-core, maybe we can call that end-to-end testing:
@sreuland If you're saying this type of test should happen in a separate repo, I agree. I think it's going to slow down development in this repo for the reasons described in comment. Especially as long as stellar-core is a component these tests are dependent on.
@tsachiherman @graydon @ire-and-curses See @sreuland's comment above. We should discuss effort to setup a QA style end-to-end test repo that runs all components of the entire Soroban experience with a test of test vectors as a top-down view of compatibility.
-
Integration testing from the sense of outside-in testing the CLI using real wasm files:
I think we can do that in this repo. The performance cost is much smaller. Incompatibilities are much easier and faster to resolve. It would be faster to iterate on code protected by these types of tests than the tests in (1). We've found this model work well for the env repo, although I acknowledge it is not perfect.
-
Integration testing from the sense of outside-in with mocked inputs:
I think we can do that in this repo, although I think (2) may be easier for us to implement in the short term because mocking will make it harder to change the overall design, and with CLI: Redesign #197 there might be a lot of changes.
@2opremio I think it's fine if you want to continue with exploring an integration test in this repo that uses quickstart in the interim until the above idea is worked out, but I think it is important that:
- Any test dependent on a running rpc server should be skipped by default. Typically skipping tests are done via a feature as I describe here Add first integration test #220 (comment).
- That the existing integration tests in the tests/ repo aren't skipped as part of ☝🏻.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Regarding @sreuland's idea for a test repo, that's captured in https://github.com/stellar/jump-cannon-deliverables/issues/25 that he just opened recently.
.github/workflows/rust.yml
Outdated
# integration tests | ||
# TODO: we should probably manage the container in the test itself | ||
# TODO: do a proper functional check on the container startup instead of waiting 15 seconds | ||
- run: docker run -d -p 8000:8000 stellar/quickstart:soroban-dev --standalone --enable-soroban-rpc --protocol-version 20 && sleep 15 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While this will work today, adding a dependency on stellar-core to the CLI tests will probably prevent many PRs from being merged on this repo. That's because stellar-core is rarely updated and released with the new version of the env interface, but the env interface is under active development, so any time the env interface is updated without also getting the core team to build a new deb, the tests dependent on the quickstart will break.
I imagine we need to iterate on this test setup, so could we move the quickstart tests into a separate job that won't block PRs from merging?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We've been running integration tests on Horizon (which depend on Core) for more than a year without many problems. They break once in a while but it hasn't been a problem IMO.
Regarding the env
changes, bear in mind this is a very basic test. Do you expect the env to break deploying and invoking a basic contract?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think in the future once Soroban is released the experience of Horizon with Core will be the same here too.
There's two parts that will causes breakages in the less stable world we're in temporarily:
-
XDR – The env could have XDR that is completely incompatible with the stellar-core. We're making breaking changes to the XDR, which is why FutureNet is an all-bets-are-off-can-be-reset-at-any-time environment.
-
Env Meta Version – Whenever the env interface changes, a version number gets updated inside the env which bubbles up into the Host, SDK, CLI, and stellar-core. If a contract is built with a different version as the environment it is running in (CLI, stellar-core), the Host will refuse to run the contract. In the future the Host will support ranges of versions, but today it is an exact match.
The second dot point will only impact tests with complete contracts, like .wasm files. The first thing could impact other tests too.
.github/workflows/rust.yml
Outdated
- run: cargo test --bins --target ${{ matrix.target }} | ||
# integration tests | ||
# TODO: we should probably manage the container in the test itself | ||
# TODO: do a proper functional check on the container startup instead of waiting 15 seconds |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can probably get something equivalent with a little bash. This is what we use in other projects:
https://github.com/stellar/starlight/blob/919096552e44eec5ec3fd6278922396926239874/.github/workflows/sdk.yml#L59-L62
.github/workflows/rust.yml
Outdated
# integration tests | ||
# TODO: we should probably manage the container in the test itself | ||
# TODO: do a proper functional check on the container startup instead of waiting 15 seconds | ||
- run: docker run -d -p 8000:8000 stellar/quickstart:soroban-dev --standalone --enable-soroban-rpc --protocol-version 20 && sleep 15 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we're going to use the quickstart image in integration/e2e tests, we should enable artificial acceleration mode. You can enable it by appending this option: --enable-core-artificially-accelerate-time-for-testing
. It will close a ledger every 1 second instead of every 5 seconds.
(Artificial acceleration made a big difference to the tests of the Starlight project where we do the same thing: https://github.com/stellar/starlight/blob/main/.github/workflows/sdk.yml#L59-L62.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We also use it in Horizon. I will add it although I think it's an early optimization (the current test doesn't take that long)
I am using the prefix to separate them when invoking them from the github actions (see |
There are integration tests in the tests/ repo that we want to run all the time, like the existing integration test in there. If there are tests you want to run only sometimes, a feature switch is a common way of doing that. See this list of options here: #220 (comment). I think we should explore running all the tests all the time, and add a env var to point it to an external one if needed. Either that or a feature switch would work. |
I want to run them in every PR, but run them separately, so that we can clearly see whether it's integration tests that are failing. |
We can already do that, it's just a matter of starting |
67e848d
to
515f50f
Compare
515f50f
to
748a8a6
Compare
I gave a try to using a feature flag for e2e tests. However, it causes everything to rebuild, making testing super slow. If I later run unit tests (without the feature flag), then everything is rebuilt again. It seems like Rust is not smart about feature flags, causing any changes to trigger a full rebuild. This breaks the build cache and adds latency to the development loop. I think I will go back to using test names for filtering tests. |
TIL you can unignore ignored tests, that's a very nice way of doing it without requiring a feature. 👏🏻 I don't think I've seen anyone do that before, but the fact that it removes the need for the feature but provides the same capabilities is awesome. (I do not see rebuilds when I run with and without the feature at commit d56e59a. The rebuilds could be to do with a few other unrelated things. If you're using rust-analyzer with VSCode, it can sometimes be a good idea to tell rust-analyzer to use a different |
@@ -49,6 +49,23 @@ jobs: | |||
- run: cargo clippy --all-targets --target ${{ matrix.target }} | |||
- run: cargo test --target ${{ matrix.target }} | |||
|
|||
e2e-test: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI this test won't be required because it isn't included in the complete
job's required jobs, which I think is good for now because this test will probably be brittle.
What
It tests deploying and invoking a contract against the RPC server
Why
There are no integration tests. Also, see stellar/quickstart#72
Known limitations
The way in which the quickstart container is spawned could be nicer (it may be a good idea to do it from the tests themselves).