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

add +nobuild flags to all relevant test cases #8934

Merged
merged 1 commit into from
Mar 19, 2021

Conversation

alessio
Copy link
Contributor

@alessio alessio commented Mar 19, 2021

CLI tests must be excluded by race detection due
to a well-known Tendermint issue.

Closes: #8923


Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

  • Targeted PR against correct branch (see CONTRIBUTING.md)
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Code follows the module structure standards.
  • Wrote unit and integration tests
  • Updated relevant documentation (docs/) or specification (x/<module>/spec/)
  • Added relevant godoc comments.
  • Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
  • Re-reviewed Files changed in the Github PR explorer
  • Review Codecov Report in the comment section below once CI passes

@alessio alessio force-pushed the alessio/norace-auth-cli-test branch from 89755b8 to 1fe0ee1 Compare March 19, 2021 04:46
@orijbot
Copy link

orijbot commented Mar 19, 2021

@orijbot
Copy link

orijbot commented Mar 19, 2021

@alessio alessio marked this pull request as ready for review March 19, 2021 04:46
@alessio alessio added A:automerge Automatically merge PR once all prerequisites pass. backport/0.42.x (Stargate) labels Mar 19, 2021
Copy link
Member

@tac0turtle tac0turtle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks fine. Can you point out what the "well-known issue in tendermint" is? Do we have an issue for this?

@fdymylja
Copy link
Contributor

--- FAIL: TestIntegrationTestSuite/TestBroadcastTx_GRPCGateway (1.03s)
      --- FAIL: TestIntegrationTestSuite/TestBroadcastTx_GRPCGateway/valid_request (0.00s)
          service_test.go:454: 
              	Error Trace:	service_test.go:454
              	            				suite.go:77
              	Error:      	Not equal: 
              	            	expected: 0x0
              	            	actual  : 0x13
              	Test:       	TestIntegrationTestSuite/TestBroadcastTx_GRPCGateway/valid_request
              	Messages:   	rawlog%!(EXTRA string=)

^ tests failing are unrelated to this PR, I've seen this fail quite frequently in the past weeks. We probably should open a separate issue for this.

@mergify mergify bot merged commit 7b09f95 into master Mar 19, 2021
@mergify mergify bot deleted the alessio/norace-auth-cli-test branch March 19, 2021 11:24
@orijbot
Copy link

orijbot commented Mar 19, 2021

@orijbot
Copy link

orijbot commented Mar 19, 2021

mergify bot pushed a commit that referenced this pull request Mar 19, 2021
Closes: #8923
(cherry picked from commit 7b09f95)

# Conflicts:
#	x/authz/client/cli/tx_test.go
#	x/authz/client/rest/grpc_query_test.go
@alessio
Copy link
Contributor Author

alessio commented Mar 19, 2021

looks fine. Can you point out what the "well-known issue in tendermint" is? Do we have an issue for this?

Look at the package testutil/network. Running two Tendermint instances at the same time makes the tests panic.

@tac0turtle
Copy link
Member

Is there an issue for this in tendermint? if not, want to open it?

alessio pushed a commit that referenced this pull request Mar 19, 2021
Closes: #8923
(cherry picked from commit 7b09f95)
Co-authored-by: Alessio Treglia <alessio@tendermint.com>
@alexanderbez
Copy link
Contributor

looks fine. Can you point out what the "well-known issue in tendermint" is? Do we have an issue for this?

Look at the package testutil/network. Running two Tendermint instances at the same time makes the tests panic.

This is already well known and I've documented it: https://github.com/cosmos/cosmos-sdk/blob/master/testutil/network/doc.go#L16-L20

@robert-zaremba
Copy link
Collaborator

Thanks for the link.

BTW:

Package network implements and exposes a fully operational in-process Tendermint test network that consists of at least one or potentially many validators. This test network can be used primarily for integration tests or unit test suites.

This doesn't sound like anything close to unit tests 😅

@alessio
Copy link
Contributor Author

alessio commented Mar 20, 2021

Thanks for the link.

BTW:

Package network implements and exposes a fully operational in-process Tendermint test network that consists of at least one or potentially many validators. This test network can be used primarily for integration tests or unit test suites.

This doesn't sound like anything close to unit tests

Yes, that comment needs to be updated 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A:automerge Automatically merge PR once all prerequisites pass.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

data race detected
8 participants