boltlabs-inc / zeekoe

Zero-knowledge layer-2 payment channels
MIT License
24 stars 1 forks source link

Add initial e2e testing infrastructure #361

Closed marsella closed 2 years ago

marsella commented 2 years ago

This adds the basic testing harness to run our end-to-end tests. It makes two large sweeps of changes:

Restructures zkchannels architecture

Before, the zkchannels protocol logic was encoded as a binary crate, which used several libraries. Now, the bulk of the protocol logic has been moved to the zkchannel library, which releases a public API to call run on each command line operation.

The Show and List command line operations were also modified to return content (e.g., details about a particular channel), rather than printing it to stdout. To support this, I added a PublicChannelDetails struct for each party, which encodes the reasonable details about a channel to release to the user.

Creates testing harness

Creates a custom integration test harness (following the advice of a blog post to configure and run a customer watcher and merchant server, execute all tests, and close down the long-running tasks.

Tests are specified as a set of operations and their expected outcome. If the servers do not reflect the expected outcome after an operation completes, the test fails. The expected outcome currently consists of the channel status according to each customer and an indicator that a process threw an error. This can be expanded as additional operations with more complex features are added (see, e.g. #350, #352).

I monitor errors thrown by the servers by writing them to a file and tagging them (via tracing) with which server created them. The current implementation deletes the log after each test (to avoid getting mixed up with errors thrown by a previous test). I hard-coded a pause (see wait_time) after certain operations to make sure the servers have enough time to finish their business. This seems like it could be a pain point in the future, but it works for the current (limited) set of tests.

This is currently hard-coded for the Tezos specification. Note that the Tezos integration cannot support parallelized operations; these tests are run serially by default.

Specific points for reviewers

I made a bunch of new issues (#350 - 360, collected under #69) to describe the remaining work necessary to get a fully working test harness. If some of these should be incorporated into this PR, please note in review.

I originally made a main and a common file that I expected to be separated roughly along Tezos/non-Tezos lines. I am not sure the division I ended up with makes sense any more. Any thoughts on organization of these? I am considering just sticking everything into one massive file and trying to make a reasonable abstraction later, when we have something to abstract over. It may also be more reasonable to put the test specs themselves in a separate file, because I think there will be a lot of them.

The CI for pull requests does not run these tests by default. Should I try to find a way to manually trigger them (just to prove this works)?

Closes #133 Closes #345 - this is no longer necessary Addresses #69, #338 but does not complete them. Addresses some tasks in #360

marsella commented 2 years ago

Hmm, I thought the change to the yml file would not run these tests, but apparently it does. I will look into fixing that (they failed because the server infrastructure is not set up).

gijsvl commented 2 years ago

Although the e2e github action seems to succeed, there is not much evidence about the tests, it would be good if there was some output about the success of the tests. Similar to regular tests.

marsella commented 2 years ago

Thanks for triggering them correctly. I think I have fixed the config issues and printed output correctly now. Also, I updated the integration test crate to cleanly return an error code instead of panicking.

For reference, I leaned that there are 5 different test targets (--lib, --bins, --tests, --examples, and --benches). Specifying --all-targets runs all of them. The behavior we want is to run library and binary tests at pull, and everything in the e2e tests. Before, I was incorrectly specifying --all-targets --lib at pull, which just ran everything instead of only library tests.

Will we have to update the yml file before merging to un-specify e2e tests at pull request?

marsella commented 2 years ago

Hmm, the changes in 37615c2 didn't fix the output like I thought they would. I get a bunch of info when I run it locally. I am not actually sure whether it is running integration_tests right now, it doesn't even have the line Running integration_tests/main.rs that I expected to see.

gijsvl commented 2 years ago

I just noticed some code for TestLogs in src/lib.rs, can we either move this somewhere else or mark it as test code such that it doesn't get included in a release build?

marsella commented 2 years ago

I just noticed some code for TestLogs in src/lib.rs, can we either move this somewhere else or mark it as test code such that it doesn't get included in a release build?

I am not sure this is possible. I think that integration tests don't use any of the code that compiles with #[cfg(test)] because they are supposed to test the public API. I can rename this to something else, maybe just Logs. The purpose of this section is to make it easier to search the logs in the tests for a specific progress message.