feat: add CLI tests #216

willemneal · 2023-02-17T20:48:28Z

Currently the docs have example cli commands for the examples and this provides tests to ensure that the docs are accurate.

gitpod-io · 2023-02-17T20:48:42Z

willemneal · 2023-02-22T19:02:49Z

test/cli-tests/tests/it/auth.rs

+
+const WASM: &Wasm = &Wasm::Release("soroban_auth_contract");
+
+const ACC1: &str = "GA6S566FD3EQDUNQ4IGSLXKW3TGVSTQW3TPHPGS7NWMCEIPBOKTNCSRU";


Please write out. No need to be so terse in an example test.

done in d6bf4f8

paulbellamy · 2023-02-24T17:27:55Z

test/cli-tests/Cargo.toml

+
+[dev-dependencies]
+predicates = "2.1.5"
+soroban-test = {  git = "https://github.com/ahalabs/soroban-tools", rev = "bc1d008" }


This should probably be the stellar repo, right?

Yes, relevant PR is here:

feat: soroban-test crate stellar-cli#437

We can wait to merge these examples tests until the soroban-tools PR is merged.

paulbellamy · 2023-02-24T17:28:46Z

The tests themselves look fine. But they feel like cli tests, so it's odd they're in the examples repo. Feels like they should be in the soroban-tools repo.

tomerweller · 2023-02-24T19:13:33Z

I agree with @paulbellamy that these tests don't naturally fit in the examples repo. I think that in a perfect world they would be in the docs repo, as the docs are what is actually being tested. However, the docs don't currently have machinery to facilitate that. So for now having them here sounds like a reasonable compromise to me.

I suggest the following:

rename cli-tests to docs-tests or docs-cli-tests to reflect the fact that we're testing docs, not the cli
add a short README to this new tests package to explain what it does
add a rustdoc at the top of every test file that links to the relevant doc it's testing

Thoughts @willemneal @chadoh?

willemneal · 2023-02-24T19:16:17Z

I think doc-tests is a good name for now. Ideally we would make the examples a git submodule and then the docs repo could host the tests there.

tomerweller · 2023-02-24T19:28:59Z

I do wonder if there's a more ergonomic way to write these tests. Constructing these commands in code is somewhat painful and will be painful to update.

Is there a way to use the cli's parser to parse a single command string instead of constructing it by code? I think this will make writing and updating these tests easier - just copy&paste the cli command.

chadoh · 2023-02-24T20:03:55Z

@tomerweller Willem and I would rather have the ability to move somewhat in the other direction.

feat: reorg into a proper library stellar-cli#443

^ this would allow us to:

depend on a specific version of the CLI
write Rust tests that execute Rust code, rather than calling out to an external CLI process
speed up the tests

What do you think?

chadoh · 2023-02-24T20:17:10Z

I suggest the following:

rename cli-tests to docs-tests or docs-cli-tests to reflect the fact that we're testing docs, not the cli

add a short README to this new tests package to explain what it does

add a rustdoc at the top of every test file that links to the relevant doc it's testing

All done.

Looks like someone needs to approve running the workflow so tests actually run.

tomerweller · 2023-02-24T22:23:49Z

write Rust tests that execute Rust code, rather than calling out to an external CLI process

I totally agree about writing tests in rust and not calling an external CLI process.

My question/wonder was whether there's a way in code to construct the cli command, using the library, instead of "manually" constructing it.

So instead of this

TestEnv::with_default(|e| {
e.new_cmd("contract")
  .arg("invoke")
  .arg("--wasm")
  .arg(&WASM.path())
  .args(["--id", "1"])
  .args(["--fn", "increment"])
  .args(["--"])
  .args(["--incr", "5"])
  .assert()
  .stderr("")
  .stdout("5\n");
});

One could potentially write something more "similar" to this:

TestEnv::with_default(|e| {
  e.new_cmd_pasre("contract invoke --wasm {} --id 1 --fn increment -- --incr 5", &WASM.path())
    .assert()
    .stderr("")
    .stdout("5\n");
});

This makes an assumption that there's an available parse function (or that one could be exposed) in the cli lib. Intuitively I think there should be one because the soroban-cli indeed parses commands from the cli that take that shape (but my intuition is often wrong). What's the feasibility of this?

willemneal · 2023-02-25T18:25:58Z

The idea would be to allow constructing an invoke, or any command, and then call a run function that returns the output.

We have tests in the cli to handle testing cli parsing but we can skip that part and have proper types.

tsachiherman · 2023-03-06T16:18:00Z

test/doc-tests/Cargo.toml

+version = "0.0.0"
+authors = ["Stellar Development Foundation <info@stellar.org>"]
+license = "Apache-2.0"
+edition = "2021"


not an expert, but shouldn't this be 2023 ?

Is there a 2023 edition (yet)? I don't see it listed in the edition guide.

tsachiherman · 2023-03-06T16:20:02Z

test/doc-tests/Cargo.toml

+
+[dev-dependencies]
+predicates = "2.1.5"
+soroban-test = {  git = "https://github.com/ahalabs/soroban-tools", rev = "698479c" }


shouldn't this be pointed to an official release ?

Yes, see related discussion from last week

This now points at an official release! Please mark this comment resolved.

chadoh · 2023-04-05T16:50:22Z

@tomerweller @tsachiherman @paulbellamy this now uses the v0.7.1 versions of both soroban-cli and soroban-test. I'd love another round of review.

soroban-test's "new way" of doing this is a little frustrating still, because soroban-cli does not allow running commands in a consistent way. @willemneal's thought on how to fix this is to make a generic async run function that all commands implement, so that the tests can be written in a consistent asynchronous way. This would also allow us to make the tests run against an RPC node down the line, potentially running the exact same tests and only setting an environment variable.

However, even without this, we believe that these tests are a great start. I think it's time to merge this PR; we can open new ones to update these tests as the testing library and CLI evolve.

paulbellamy

LGTM. It would be nice to have these run as part of CI

This makes use of the newly-published [soroban-test](https://docs.rs/soroban-test/latest/soroban_test/) crate and the newly-reorganized [soroban-cli](https://docs.rs/soroban-cli) library to test all commands shown in the docs. This gives us confidence that all of these commands work, though it does come with some downsides: - these tests live far from the thing they are meant to test - could we generate the docs from this repository, rather than keeping them in sync with each other? - maybe, but it's a little unclear how that would work - this is a good starting point Another shortcoming of the approach here: it still makes use of `assert_cmd` in some tests, which runs the version of `soroban-cli` installed on the host system, making it quite brittle. This is necessary because some commands (`Cmd`s) from `soroban-cli` do not currently operate in a test-friendly way. This can be seen in a secondary way, too: not all `Cmd`s can be run the same way. Some use `run`, some use `run_in_sandbox`, some take arguments, some do not. These concerns can all be addressed together by refactoring `soroban-cli` to make all `Cmd`s implement an asynchronous `run` method with the same signature, and which will return a struct containing both `stdout` and `stderr`. However, even with these shortcomings and downsides, it is still worth merging these tests as-is, since it will give us higher confidence that everything works as expected with the current code. We can refactor these tests later, and potentially move them to a different repo. Co-authored-by: Chad Ostrowski <221614+chadoh@users.noreply.github.com>

chadoh · 2023-04-11T15:56:30Z

@paulbellamy can you approve the workflow to run?

paulbellamy · 2023-04-12T11:32:44Z

Nope, I don't have write access to this repo. Maybe @leighmcculloch can?

tsachiherman · 2023-04-12T12:12:29Z

Just did. Could you address the conflicts ?

* main: Add a very simple account contract example. (stellar#227)

paulbellamy · 2023-04-21T13:05:45Z

@chadoh Seems like the test runner is failing to launch the soroban cli. Is it actually being built where expected?

Edit: Seems like the .github/workflows/rust.yml probably needs to cargo install --locked soroban-cli before running the tests. Good luck getting it to install the version matching whats in test/doc-tests tho 🤷

paulbellamy · 2023-04-24T17:30:46Z

Aaaaaaand the top-level Cargo.toml was removed, so there's no more make build-test-wasms, and this is broken now. :(

This was referenced Feb 21, 2023

feat: add CLI tests AhaLabs/soroban-examples#1

Closed

feat: soroban-test crate stellar/stellar-cli#437

Closed

willemneal commented Feb 22, 2023

View reviewed changes

chadoh force-pushed the feat/tests branch from 3e63695 to d361c2d Compare February 23, 2023 17:10

willemneal marked this pull request as ready for review February 24, 2023 15:33

paulbellamy approved these changes Feb 24, 2023

View reviewed changes

paulbellamy reviewed Feb 24, 2023

View reviewed changes

chadoh force-pushed the feat/tests branch 2 times, most recently from ad807ac to 3d5998e Compare February 27, 2023 21:08

tsachiherman reviewed Mar 6, 2023

View reviewed changes

chadoh force-pushed the feat/tests branch 6 times, most recently from 39b8810 to 27cacd6 Compare March 29, 2023 18:00

chadoh mentioned this pull request Mar 29, 2023

fix: parse XDR Result to JSON; encode RawVal return types stellar/stellar-cli#541

Merged

chadoh force-pushed the feat/tests branch 3 times, most recently from 4976a35 to e2b9d7b Compare March 29, 2023 19:33

chadoh force-pushed the feat/tests branch from 1bbeb0e to 0d977e1 Compare April 5, 2023 16:46

paulbellamy approved these changes Apr 6, 2023

View reviewed changes

chadoh force-pushed the feat/tests branch 3 times, most recently from 9a3b573 to e3f9532 Compare April 6, 2023 19:15

chadoh force-pushed the feat/tests branch from e3f9532 to 2de3f37 Compare April 6, 2023 20:33

Merge remote-tracking branch 'origin/main' into feat/tests

12336bb

Merge branch 'main' into feat/tests

73033d7

* main: Add a very simple account contract example. (stellar#227)

chadoh force-pushed the feat/tests branch from b763315 to 73033d7 Compare April 13, 2023 14:18

update soroban_token_spec.wasm

4878efb

chadoh mentioned this pull request Apr 13, 2023

build: update to same revs as soroban-examples stellar/stellar-cli#585

Merged

tomerweller approved these changes Apr 19, 2023

View reviewed changes

fix: cargo fmt

ba17ec1

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add CLI tests #216

feat: add CLI tests #216

willemneal commented Feb 17, 2023 •

edited

Loading

gitpod-io bot commented Feb 17, 2023

willemneal Feb 22, 2023

chadoh Feb 23, 2023

paulbellamy Feb 24, 2023

chadoh Feb 24, 2023

paulbellamy commented Feb 24, 2023

tomerweller commented Feb 24, 2023

willemneal commented Feb 24, 2023

tomerweller commented Feb 24, 2023

chadoh commented Feb 24, 2023

chadoh commented Feb 24, 2023

tomerweller commented Feb 24, 2023 •

edited

Loading

willemneal commented Feb 25, 2023

tsachiherman Mar 6, 2023

chadoh Apr 3, 2023

tsachiherman Mar 6, 2023

chadoh Mar 6, 2023

chadoh Apr 5, 2023

chadoh commented Apr 5, 2023

paulbellamy left a comment

chadoh commented Apr 11, 2023

paulbellamy commented Apr 12, 2023

tsachiherman commented Apr 12, 2023

paulbellamy commented Apr 21, 2023 •

edited

Loading

paulbellamy commented Apr 24, 2023 •

edited

Loading


		const WASM: &Wasm = &Wasm::Release("soroban_auth_contract");

		const ACC1: &str = "GA6S566FD3EQDUNQ4IGSLXKW3TGVSTQW3TPHPGS7NWMCEIPBOKTNCSRU";

feat: add CLI tests #216

Are you sure you want to change the base?

feat: add CLI tests #216

Conversation

willemneal commented Feb 17, 2023 • edited Loading

gitpod-io bot commented Feb 17, 2023

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

paulbellamy commented Feb 24, 2023

tomerweller commented Feb 24, 2023

willemneal commented Feb 24, 2023

tomerweller commented Feb 24, 2023

chadoh commented Feb 24, 2023

chadoh commented Feb 24, 2023

tomerweller commented Feb 24, 2023 • edited Loading

willemneal commented Feb 25, 2023

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

chadoh commented Apr 5, 2023

paulbellamy left a comment

Choose a reason for hiding this comment

chadoh commented Apr 11, 2023

paulbellamy commented Apr 12, 2023

tsachiherman commented Apr 12, 2023

paulbellamy commented Apr 21, 2023 • edited Loading

paulbellamy commented Apr 24, 2023 • edited Loading

willemneal commented Feb 17, 2023 •

edited

Loading

tomerweller commented Feb 24, 2023 •

edited

Loading

paulbellamy commented Apr 21, 2023 •

edited

Loading

paulbellamy commented Apr 24, 2023 •

edited

Loading