Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[ABCI]: catch up with 0.16.2 / 0.33.4 #233

Closed
liamsi opened this issue Apr 21, 2020 · 3 comments · Fixed by #238
Closed

[ABCI]: catch up with 0.16.2 / 0.33.4 #233

liamsi opened this issue Apr 21, 2020 · 3 comments · Fixed by #238
Assignees
@liamsi

Comments

@liamsi
Copy link
Member

liamsi commented Apr 21, 2020

There seems to be a minor glitch in the integration tests:

failures:

---- rpc::abci_info stdout ----
thread 'rpc::abci_info' panicked at 'assertion failed: `(left == right)`
  left: `"0.16.2"`,
 right: `"0.16.1"`', tendermint/tests/integration.rs:37:9
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace


failures:
    rpc::abci_info

test result: FAILED. 9 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out

error: test failed, to rerun pass '-p tendermint --test integration'
##[error]The process '/usr/share/rust/.cargo/bin/cargo' failed with exit code 101
@liamsi
Copy link
Member Author

liamsi commented Apr 21, 2020

Discovered in #232

@liamsi
Copy link
Member Author

liamsi commented Apr 21, 2020

Status: Downloaded newer image for tendermint/tendermint:latest

Ah, this is due to the latest changes in 0.33.4: https://github.com/tendermint/tendermint/blob/v0.33/CHANGELOG.md#v0334

That's why it started failing. The changes are fresh (release 5h ago).

@liamsi liamsi changed the title Integration tests fail ABCI: catch up with 0.16.2 / 0.33.4 Apr 22, 2020
@liamsi liamsi changed the title ABCI: catch up with 0.16.2 / 0.33.4 [ABCI]: catch up with 0.16.2 / 0.33.4 Apr 22, 2020
@liamsi liamsi self-assigned this Apr 24, 2020
@liamsi
Copy link
Member Author

liamsi commented Apr 24, 2020 •
edited
Loading

We can simply bump the version to fix this particular issue. But this reveals that the abci types and tests are out of date. Instead of manually keeping track of the types under abci, we should rather merge one of the existing rust abci repos in here or at least generate the rust structs from the .proto files instead.

Related issues:

This was referenced Apr 24, 2020
Merged
Merged
Merged
@liamsi liamsi mentioned this issue May 2, 2020
Closed
@liamsi liamsi mentioned this issue May 27, 2020
Closed
shonfeder pushed a commit to shonfeder/tendermint-rs that referenced this issue Jun 15, 2020
Remove test for hardcoded ABCI version
b452fcc 
Proposes one solution to the underlying issue behind

- 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 the abci version is within a
version range which is 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.

The current approach of hardcoding in a version in the integration test
tiself seems to create a lot of busy work due to uninformative test
failures, and I'm not sure if delivers value. If the integration tests
are meant to test that the RPC client integrates correctly with ACBI,
should we really consider that integration has failed if everything
works as expected while interfacing with an older (or newer) version?
shonfeder pushed a commit to shonfeder/tendermint-rs that referenced this issue Jun 15, 2020
Remove test for hardcoded ABCI version
adc3101 
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>
shonfeder pushed a commit to shonfeder/tendermint-rs that referenced this issue Jun 15, 2020
Remove test for hardcoded ABCI version
3b04f31 
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>
shonfeder pushed a commit to shonfeder/tendermint-rs that referenced this issue Jun 15, 2020
Remove test for hardcoded ABCI version
5585b41 
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>
liamsi pushed a commit that referenced this issue Jun 15, 2020
Add integration test pinned to tendermint-go v0.33.5 (#324)
704e244 
* Add integration test pinned to tendermint-go v0.33

Closes #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>

* Remove test for hardcoded ABCI version

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.,

- #249
- #238
- #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>

* Update changelog

Signed-off-by: Shon Feder <shon@informal.systems>
@greg-szabo greg-szabo mentioned this issue Aug 4, 2020
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@liamsi liamsi

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

bump abci version in integration tests informalsystems/tendermint-rs
1 participant
@liamsi