-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
fix: --max-msgs
with generate-only
should not throw error
#10842
Conversation
--max-msgs
with generate-only
should throw error--max-msgs
with generate-only
should not throw error
…k into ap/max-msgs-all-rewards
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm overall, we should add a changelog entry
…k into ap/max-msgs-all-rewards
@@ -19,3 +20,9 @@ func TestIntegrationTestSuite(t *testing.T) { | |||
func TestGRPCQueryTestSuite(t *testing.T) { | |||
suite.Run(t, new(GRPCQueryTestSuite)) | |||
} | |||
|
|||
func TestWithdrawAllSuite(t *testing.T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a new suite here, since there are few tests are failing if I change NumValidators = 2
in existing integration suite (seems like non-determinism) and couldn't test them on local machine since the tests are keep on throwing timeout error.
9e9c3e4
to
fac4cad
Compare
c903a9b
to
bf2d705
Compare
@@ -709,7 +727,7 @@ func (s *IntegrationTestSuite) TestGetCmdSubmitProposal() { | |||
invalidPropFile.Name(), | |||
fmt.Sprintf("--%s=%s", flags.FlagFrom, val.Address.String()), | |||
fmt.Sprintf("--%s=true", flags.FlagSkipConfirmation), | |||
fmt.Sprintf("--%s=%s", flags.FlagBroadcastMode, flags.BroadcastSync), | |||
fmt.Sprintf("--%s=%s", flags.FlagBroadcastMode, flags.BroadcastBlock), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated this (made changes accordingly), because some times tests are getting timeout with BroadcastSync
.
@alexanderbez can you have another look please, few changes have been made after your approval. Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changes lgtm, but the non-determinism/timeouts seems a bit worrying. If we merge this, can we maybe create an issue to track this?
ping @alexanderbez, would you have bandwidth to take another look at this PR? |
Are you referring to the simulation non-determinism? If so, yeah that's very worrying and I would hestitate to merge this PR with those failing. But why does a CLI change impact those? |
No, this is not about simulations non-determinism, I was mentioning in integration tests itself there were few tests continuously producing different delegation rewards all the time. |
That shouldn't be possible unless there is variance in how many blocks we wait between txs and/ queries. |
Description
Closes: #10841
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!
to the type prefix if API or client breaking changeCHANGELOG.md
Reviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
I have...
!
in the type prefix if API or client breaking change