Add integration test pinned to tendermint-go v0.33.5 #324

shonfeder · 2020-06-14T22:33:01Z

Closes #304

With this change, CI runs two integration tests:

A stable test to protect against regressions, run against a pinned
version of the tendermint/tendermint docker image.
A latest test to track whether we're maintaining parity with the
latest development version of tendermint-go.

IIUC, we'd ideally like (1) to be a required test, and to get visibility on (2) but not block the PR if it fails. I can configure (2) so that it the job succeeds even if the step fails, but then we lose clear visibility on the failure.

Please see individual commits for more detail.

Referenced an issue explaining the need for the change
Updated all relevant documentation in docs
Updated all code comments where relevant
Wrote tests
Updated CHANGES.md

codecov-commenter · 2020-06-14T22:43:59Z

Codecov Report

Merging #324 into master will not change coverage.
The diff coverage is n/a.

@@          Coverage Diff           @@
##           master    #324   +/-   ##
======================================
  Coverage    30.5%   30.5%           
======================================
  Files         127     127           
  Lines        4581    4581           
  Branches     1416    1416           
======================================
  Hits         1398    1398           
  Misses       2216    2216           
  Partials      967     967

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 740cb66...ec50545. Read the comment docs.

shonfeder · 2020-06-15T01:18:31Z

tendermint/tests/integration.rs

@@ -35,7 +35,6 @@ mod rpc {
    async fn abci_info() {
        let abci_info = localhost_rpc_client().abci_info().await.unwrap();

-        assert_eq!(&abci_info.version, "0.17.0");


See the specific commit message for my reasoning here: b452fcc

An important caveat (tho not well placed in the git logs), is that I don't have a very informed position, given that this is my first tiny foray into helping here, so please forgive if my suggestions are wide of the mark.

Some alternatives that might make sense, in addition to the approach I propose in the commit message:

Specify the expected version via an environment variable during testing (allowing us to support testing against multiple versions, as desired).

Change this to a check for a string that matches a semantic version (e.g., \d+\.\d+\.\d+)

shonfeder · 2020-06-15T01:49:09Z

This change should make it possible to make the test-integration-stable test required (if that's desirable).

shonfeder · 2020-06-15T01:55:35Z

.github/workflows/rust.yml

@@ -61,11 +61,34 @@ jobs:
          command: test
          args: --all-features --no-fail-fast

-  test-integration-ignored:
+  # TODO(shonfeder): remove duplication once GitHub addresses


IIUC, we could DRY this out now if we want to define our own action for this, but I'm not sure if we want to complicate the CI config that way. I'm up for it tho, if anyone thinks it's preferable.

shonfeder · 2020-06-15T01:58:41Z

.github/workflows/rust.yml

    runs-on: ubuntu-latest
    services:
      tendermint:
-        image: tendermint/tendermint
+        image: tendermint/tendermint:v0.33.5


It would be nice to set this version somewhere more canonical, but from what I've seen of github actions so far, there's no clear way to parameterize the values of an image field here. I'd be interested in suggestions for a more elegant approach here. Failing that, I could at least document this somewhere (and the question is just where that should go).

I don't have much experience with gh either. I think even having the version more prominently in an env variable, might be ok: https://help.github.com/en/actions/configuring-and-managing-workflows/using-environment-variables

@marbar3778 as you looked a lot more into GA, do you have a suggestion how to parameterize this?

Closes informalsystems#304 Also renames test-integration-ignored to test-integration-latest. With this change, CI runs two integration tests: 1. A `stable` test to protect against regressions, run against a pinned version of the tendermint/tendermint docker image. 2. A `latest` test to track whether we're maintaining parity with the latest development version of tendermint-go. Signed-off-by: Shon Feder <shon@informal.systems>

Hardcoding a test for the version in this way makes it impossible to run the integration tests against different ABCI versions. This change proposes one solution to address this problem and to the underlying issue behind, e.g., - informalsystems#249 - informalsystems#238 - informalsystems#233 My sense is that, if we want to set a strict abci version requirement for the rpc client, then we should put that in the source code itself. E.g., we might put a check on the client that ensures the abci version is within a specified version range known to be supported. If the version is outside that range, we could either error out or log errors/warning to alert users that we don't guarantee compatibility. However, the current approach of hardcoding in a version in the integration test seems to create a lot of busy work due to uninformative test failures and it's not obvious what value it delivers. If the integration tests are meant to test that the RPC client integrates correctly with ACBI, should we really consider integration to have failed when everything works as expected while interfacing with an older (or newer) version? Signed-off-by: Shon Feder <shon@informal.systems>

Signed-off-by: Shon Feder <shon@informal.systems>

romac · 2020-06-15T08:59:46Z

This change should make it possible to make the test-integration-stable test required (if that's desirable).

Yeah I think we want to fail the build if the integration tests do not pass on stable.

Great job by the way, thanks a lot!

liamsi

Thanks @shonfeder !!!

liamsi · 2020-06-15T11:20:09Z

I'm surprised the integration tests still fail despite fixing the version to 0.33.5 🤔 maybe there were some changes we missed in the meantime.

tac0turtle · 2020-06-15T11:24:11Z

as you looked a lot more into GA, do you have a suggestion how to parameterize this?

I have not. Other than env variables I can't think of anything right now but will think about it for a bit and geet back to you.

I'm surprised the integration tests still fail despite fixing the version to 0.33.5 🤔 maybe there were some changes we missed in the meantime.

This may have something to do with our new lib for encoding json to replace amino.

liamsi · 2020-06-15T11:34:33Z

Merging as is. I'll investigate why the tests are failing in a separate pr later.

liamsi · 2020-06-15T11:49:11Z

This may have something to do with our new lib for encoding json to replace amino.

This is actually because the tests against :latest are still running:

From the introductory comment:

I can configure (2) so that it the job succeeds even if the step fails, but then we lose clear visibility on the failure.

Yes, I think that would be OK for now! Please do (or anyone else with necessary perms)

shonfeder commented Jun 15, 2020

View reviewed changes

shonfeder force-pushed the pin-integration-test-to-v0.33 branch from b452fcc to adc3101 Compare June 15, 2020 01:26

shonfeder changed the title ~~WIP: Add job pinned to tendermint-go v0.33.5~~ Add integration test pinned to tendermint-go v0.33.5 Jun 15, 2020

shonfeder force-pushed the pin-integration-test-to-v0.33 branch from adc3101 to e2c444a Compare June 15, 2020 01:54

shonfeder requested review from greg-szabo, brapse and liamsi June 15, 2020 01:56

shonfeder commented Jun 15, 2020

View reviewed changes

Shon Feder added 3 commits June 14, 2020 22:01

Update changelog

ec50545

Signed-off-by: Shon Feder <shon@informal.systems>

shonfeder force-pushed the pin-integration-test-to-v0.33 branch from e2c444a to ec50545 Compare June 15, 2020 02:01

liamsi approved these changes Jun 15, 2020

View reviewed changes

liamsi merged commit 704e244 into informalsystems:master Jun 15, 2020

shonfeder mentioned this pull request Jun 15, 2020

Set integration-test-latest to continue_on_error #325

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add integration test pinned to tendermint-go v0.33.5 #324

Add integration test pinned to tendermint-go v0.33.5 #324

shonfeder commented Jun 14, 2020 •

edited

Loading

codecov-commenter commented Jun 14, 2020 •

edited

Loading

shonfeder Jun 15, 2020

shonfeder commented Jun 15, 2020

shonfeder Jun 15, 2020

shonfeder Jun 15, 2020

liamsi Jun 15, 2020

romac commented Jun 15, 2020

liamsi left a comment

liamsi commented Jun 15, 2020

tac0turtle commented Jun 15, 2020

liamsi commented Jun 15, 2020

liamsi commented Jun 15, 2020

Add integration test pinned to tendermint-go v0.33.5 #324

Add integration test pinned to tendermint-go v0.33.5 #324

Conversation

shonfeder commented Jun 14, 2020 • edited Loading

codecov-commenter commented Jun 14, 2020 • edited Loading

Codecov Report

shonfeder Jun 15, 2020

Choose a reason for hiding this comment

shonfeder commented Jun 15, 2020

shonfeder Jun 15, 2020

Choose a reason for hiding this comment

shonfeder Jun 15, 2020

Choose a reason for hiding this comment

liamsi Jun 15, 2020

Choose a reason for hiding this comment

romac commented Jun 15, 2020

liamsi left a comment

Choose a reason for hiding this comment

liamsi commented Jun 15, 2020

tac0turtle commented Jun 15, 2020

liamsi commented Jun 15, 2020

liamsi commented Jun 15, 2020

shonfeder commented Jun 14, 2020 •

edited

Loading

codecov-commenter commented Jun 14, 2020 •

edited

Loading