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.

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

RPC integration tests failing on CI #304

Closed
romac opened this issue Jun 8, 2020 · 4 comments · Fixed by #324 or #325
Closed

RPC integration tests failing on CI #304

romac opened this issue Jun 8, 2020 · 4 comments · Fixed by #324 or #325
Assignees
Labels
bug Something isn't working ci Related to continuous integration rpc serialization
Milestone

Comments

@romac
Copy link
Member

romac commented Jun 8, 2020

The RPC integration tests are currently failing on CI.

Affected PRs are:

Here's an excerpt from the build logs for #302:

Click to show log
running 11 tests

test rpc::abci_info ... ok
test rpc::abci_query ... ok
test rpc::block ... FAILED
test rpc::block_results ... ok
test rpc::commit ... FAILED
test rpc::blockchain ... FAILED
test rpc::genesis ... ok
test rpc::health ... ok
test rpc::net_info ... ok
test rpc::status_integration ... ok
test rpc::event_subscription ... FAILED

failures:

---- rpc::block stdout ----
thread 'rpc::block' panicked at 'called `Result::unwrap()` on an `Err` value: Error { code: ParseError, message: "Parse error. Invalid JSON", data: Some("invalid type: integer `1`, expected a string at line 8 column 19") }', tendermint/tests/integration.rs:73:26
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

---- rpc::commit stdout ----
thread 'rpc::commit' panicked at 'called `Result::unwrap()` on an `Err` value: Error { code: ParseError, message: "Parse error. Invalid JSON", data: Some("invalid type: integer `0`, expected a string at line 17 column 23") }', tendermint/tests/integration.rs:111:27

---- rpc::blockchain stdout ----
thread 'rpc::blockchain' panicked at 'called `Result::unwrap()` on an `Err` value: Error { code: ParseError, message: "Parse error. Invalid JSON", data: Some("invalid type: integer `1`, expected a string at line 11 column 23") }', tendermint/tests/integration.rs:95:31

---- rpc::event_subscription stdout ----
[tendermint/src/rpc/event_listener.rs:95] "We did not receive a valid JSONRPC wrapped ResultEvent!" = "We did not receive a valid JSONRPC wrapped ResultEvent!"
thread 'rpc::event_subscription' panicked at 'called `Result::unwrap()` on an `Err` value: Error { code: Other(-1), message: "Error (code: 0)", data: Some("received neither event nor generic string message") }', tendermint/tests/integration.rs:164:34

failures:
    rpc::block
    rpc::blockchain
    rpc::commit
    rpc::event_subscription

test result: FAILED. 7 passed; 4 failed; 0 ignored; 0 measured; 0 filtered out
@romac romac added bug Something isn't working serialization rpc ci Related to continuous integration labels Jun 8, 2020
@liamsi
Copy link
Member

liamsi commented Jun 8, 2020

Related comment: #120 (comment)

I'll have a look.

@liamsi
Copy link
Member

liamsi commented Jun 8, 2020

This is actually also related: #305

We first need to decide if we want to migrate and be on par with 0.34 as soon as it is released and basically keep up to date with the changes in go-tendermint's master (because the docker images we are testing against are build from master).

cc @ebuchman

@ebuchman
Copy link
Member

ebuchman commented Jun 9, 2020

Yes we should target v0.34 but we need some lead time so our tests shouldn't fail just because we're behind the latest breaking release. We should make sure the tests run against something stable ...

@brapse brapse added this to the Light Node milestone Jun 11, 2020
shonfeder pushed a commit to shonfeder/tendermint-rs that referenced this issue Jun 14, 2020
Closes informalsystems#304

Also rename test-integration-ignored to test-integration-latest.

The latest integration tracks whether we're maintaining parity with the
latest development version of tendermint-go, while the stable
integration ensures we avoid regressions.
shonfeder pushed a commit to shonfeder/tendermint-rs that referenced this issue Jun 15, 2020
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 <[email protected]>
shonfeder pushed a commit to shonfeder/tendermint-rs that referenced this issue Jun 15, 2020
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 <[email protected]>
shonfeder pushed a commit to shonfeder/tendermint-rs that referenced this issue Jun 15, 2020
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 <[email protected]>
liamsi pushed a commit that referenced this issue Jun 15, 2020
* 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 <[email protected]>

* 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 <[email protected]>

* Update changelog

Signed-off-by: Shon Feder <[email protected]>
@liamsi
Copy link
Member

liamsi commented Jun 15, 2020

They are still failing now (because tests against latest are still running and marked as required): https://github.com/informalsystems/tendermint-rs/runs/772419078#step:5:271
re-opening

@liamsi liamsi reopened this Jun 15, 2020
shonfeder pushed a commit to shonfeder/tendermint-rs that referenced this issue Jun 15, 2020
shonfeder pushed a commit to shonfeder/tendermint-rs that referenced this issue Jun 15, 2020
Closes informalsystems#304

We want to run the integration test against the latest version, but
don't want it to clutter CI with failures. Tests against the pinned
stable version still fail.

Also fixes some whitespace.

Signed-off-by: Shon Feder <[email protected]>
shonfeder pushed a commit to shonfeder/tendermint-rs that referenced this issue Jun 15, 2020
Closes informalsystems#304

We want to run the integration test against the latest version, but
don't want it to clutter CI with failures. Tests against the pinned
stable version still fail.

Also fixes some whitespace.

Signed-off-by: Shon Feder <[email protected]>
shonfeder pushed a commit to shonfeder/tendermint-rs that referenced this issue Jun 15, 2020
Closes informalsystems#304

We want to run the integration test against the latest version, but
don't want it to clutter CI with failures. Tests against the pinned
stable version still fail.

Also fixes some whitespace.

Signed-off-by: Shon Feder <[email protected]>
brapse pushed a commit that referenced this issue Jun 15, 2020
* Set integration-test-latest to continue_on_error

Closes #304

We want to run the integration test against the latest version, but
don't want it to clutter CI with failures. Tests against the pinned
stable version still fail.

Also fixes some whitespace.

Signed-off-by: Shon Feder <[email protected]>

* Update changelog

Also fixed previous entries, so they pointed to their originating issue
rather than the PR that implemented them, to be consistent with other
entries.

* fixup! Set integration-test-latest to continue_on_error
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working ci Related to continuous integration rpc serialization
Projects
None yet
6 participants