Add end-to-end testing workflow #282
Conversation
* ci: turn doc warnings into errors (#259) * remove unnecessary comments * Add script to check rust versions * removed versions and editions except from root, updated script * add back cargo make doc to ci * Added section for testing * Fixed typo * Set versions to 0.1.0 + inherit all infos from workspace * Fix use of versioning * Set individual versions for crates * Fixed nits, code organization and updated links * Improve naming of ci job * Fix formatting in contributing.md * Add task to format not only check * Reduced indent --------- Co-authored-by: Augusto Hack <[email protected]>
* Working initial docker * fmt * added script * Removed docker script + updated integration * script works * Need to install grpcurl * Docker works * Working Dockerfile * ci: turn doc warnings into errors (#259) * Removed makefile, removing start.sh file moving in Dockerfile * Added bookworm + alpine + removed gcc + added caching * Moved Dockerfile to node and added Makefile.toml * cargo make works + builds dockerfile for node * remove start script * added comment regarding PID1 * Set labels at top of Dockerfile + added comments * Added correct dependencies and explanation * Volume works persisting db files between runs * Added documentation * Moved labels + enable mounting of miden-node.toml * Added docker-run-node command + added build arguments * Add git commit as arg to docker container * Added spacing --------- Co-authored-by: Augusto Hack <[email protected]>
* ci: turn doc warnings into errors (#259) * Separated different CI jobs in their respective files * Add next branch in ci * Fix wrong naming in CI + use cargo make for doc * Fixed badge + removed unnecessary additional file * Change naming of ci and jobs * Fix naming * Fix comment --------- Co-authored-by: Augusto Hack <[email protected]>
|
Should we merge this? |
I just want to go over, cleanup and should be ready for review & merge. |
There was a problem hiding this comment.
Thank you! Looks great! I did a very quick review and left a couple of nits/questions inline. I defer a more detailed review to @Dominik1999 or @hackaugusto.
One note for the future: these end-to-end tests will work only when node and client versions are in sync. But we won't be able to use them when the node has some breaking changes which haven't been propagated to the client yet. To address this, we'll need to create a separate "test runner" which will not rely on miden-client. Let's create an issue for this.
| - name: Install cargo make | ||
| run: cargo install cargo-make | ||
| - name: cargo make - docker-build-node | ||
| run: cargo make docker-build-node | ||
| - name: cargo make - docker-run-node | ||
| run: cargo make docker-run-node | ||
| - name: cargo install - miden-client | ||
| run: cargo install miden-client --features concurrent,testing | ||
| - name: cargo make - test-end-to-end | ||
| run: cargo make test-end-to-end |
There was a problem hiding this comment.
Question: is the docker image built with concurrent,testing by default? Should we specify these features explicilty?
There was a problem hiding this comment.
Indeed the docker image is built with the concurrent,testing features enabled by default.
Could you clarify by specify these features explicitly?
There was a problem hiding this comment.
On line 46 we explicitly build miden-client with concurrent,testing features. Could we do something similar on line 42?
The main motivation is to make sure that that changing the defaults for docker build does not break this task.
| # Sync client and create tx between faucet and account | ||
| miden-client sync | ||
| miden-client tx new mint $basic_account $faucet_account 777 | ||
| sleep 15 |
There was a problem hiding this comment.
why is this needed, can we get rid of this? sleep in tests is to me a big no-no.
There was a problem hiding this comment.
The sleep is needed to leave some time for the rollup to include the transaction in a block and the block to be applied to the db. If not then the note does not have a commit height and it cannot be consumed by the Miden client.
Do you see a better solution than sleeping?
There was a problem hiding this comment.
Polling would be the best option here. Probably the miden-client should have a command similar to miden-client wait-for-note <note_id> where the client would poll until the note_id is available.
There was a problem hiding this comment.
A couple of other ways to implement this now:
- Wait for two blocks to be created before moving on (
miden-client inforeturns the current block number; should be fairly easy to parse the response and sync until you are one or two blocks above the original count) - Wait until the transaction has been committed (although there is not any super simple way to programmatically get this from the client, but you can use
tx listand check that the transaction ID is now committed)
| [tasks.test-end-to-end] | ||
| workspace = false | ||
| script = ''' | ||
| ./scripts/test-end-to-end.sh |
There was a problem hiding this comment.
I usually think of E2E as a category of tests, and not a single test. I would call this more of a smoketest. But eventually we will need something more robust to run additional tests.
There was a problem hiding this comment.
The idea is that that the bash scripts can be extended with other flows enabling more testing opportunities. For now there is only 1 indeed.
There was a problem hiding this comment.
What I was trying to say with "something more robust" is that shell script is not a good language to write E2E tests. It works for a single test like this, which does a good job of covering the happy path and being a smoke test. But once we have a handful of tests we should be able to do other things, for example:
- have test selection, similar to how we do
cargo test -- <test_name> - have tests run in parallel, similar to how
cargo testuses runners - have the ability to debug the test scripts, and here shell scripts really lose the upper hand
To get the above, we will eventually have to translate the .sh into a .rs or .py. But I don't think we have to do this now.
* ci: turn doc warnings into errors (#259) * Boilerplate done * pushed fix for miden-client * Fix formatting * Pulled after client fix * Fixed typos + added build_client fn * Added configs + figment * Fix client implementation in faucet + use released client * updated cargo * Fixed Miden node when importing Miden client, db errors * Format files * Added proper support of configuration file + logging * Improved config file handling * Removed superfluous file * First pass at improvements * Change naming of note * Fixed html + added metadata endpoint * Want to implement Display for NoteId * Upgraded js to use async + NoteId does not impl Display in main * cargo updated --------- Co-authored-by: Augusto Hack <[email protected]>
Created here: #291 |
|
This PR does not encompass what we need for end-to-end testing on the Miden node. Some of these tests are already covered by the Miden client. We will implement more robust end-to-end testing when the Miden node and general Miden crates are ready. |
In this PR I add end-to-end tests for the Miden node.
closes: #222