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

test: enable sim test, race test #326

Merged
merged 16 commits into from
Sep 27, 2021
Merged

Conversation

egonspace
Copy link

@egonspace egonspace commented Sep 15, 2021

Description

  • Deleted comment from race test of test.yml github action
  • Enabled sim test
    • I fixed some bug from the sim test failures
    • don't GetConsensusParams() while initializing checkState. Reverts a part of feat: implement validateGasWanted() (#48) #142.
    • use random valid sig block period over 100 for sim testing
    • modify BaseAccount marshal/unmarshal
    • add BaseVestingAccount, ContinuousVestingAccount, PeriodicVestingAccount and DelayedVestingAccount marshalJSONPB/unmarshalJSONPB
    • x/distribution/keeper/hooks.go address conversion bug
    • x/distribution/types/keys.go key prefix bug
    • remove param cache: this is a bug. Several Sets and no Write make the mismatch between db store and cache. Reverts chore: caching paramset #198
    • x/slashing/keeper/hooks.go address conversion bug
  • remove invalid keyworkd SUFFIX_FILTER from sims.yml
  • fix markdown check failures from docs/core/proto-docs.md

closes: #318


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

@egonspace egonspace self-assigned this Sep 15, 2021
@egonspace egonspace added this to the Ebony alpha milestone Sep 15, 2021
@egonspace egonspace added the WIP label Sep 15, 2021
@codecov
Copy link

codecov bot commented Sep 15, 2021

Codecov Report

❗ No coverage uploaded for pull request base (main@e69fb95). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #326   +/-   ##
=======================================
  Coverage        ?   53.16%           
=======================================
  Files           ?      642           
  Lines           ?    67253           
  Branches        ?        0           
=======================================
  Hits            ?    35757           
  Misses          ?    28551           
  Partials        ?     2945           

@iproudhon
Copy link
Contributor

Looking good to me, other than lint failure.

@egonspace egonspace changed the title test: enable race test test: enable sim test, race test Sep 16, 2021
@@ -361,7 +361,7 @@ func (app *BaseApp) setCheckState(header ocproto.Header) {

app.checkState = &state{
ms: ms,
ctx: ctx.WithConsensusParams(app.GetConsensusParams(ctx)),
ctx: ctx,
Copy link
Author

@egonspace egonspace Sep 23, 2021

Choose a reason for hiding this comment

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

This modification reverts a part of #142.
The previous code causes sim test failure.
We must not call app.GetConsensusParams() while we initialize checkState because app.GetConsensusParams() caches nil value in cache store.
Normally, this is not a problem, but this is a problem because simulation tests do deliverTx immediately without checkTx.

# - name: Run cosmovisor tests
# run: cd cosmovisor; make
# if: env.GIT_DIFF
# TODO: disable test-cosmovisor; this test uses uploaded binary(cosmos-sdk)
Copy link
Author

Choose a reason for hiding this comment

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

In order to enable this, research on test-cosmovisor is needed. We have to do this as a separate task.

@egonspace egonspace removed the WIP label Sep 24, 2021
@egonspace egonspace requested a review from iproudhon September 24, 2021 05:47
@@ -142,8 +141,9 @@ jobs:
run: go version
- uses: technote-space/get-diff-action@v5.0.1
with:
SUFFIX_FILTER: |
PATTERNSR: |
Copy link
Author

Choose a reason for hiding this comment

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

I found a typo. I'll fix it in next PR or next commit for a comment.

Copy link
Contributor

@whylee259 whylee259 left a comment

Choose a reason for hiding this comment

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

LGTM. I've left a minor comment.

Comment on lines 416 to 417
PubType byte `json:"pub_type"`
PubKey []byte `json:"pub_key"`
Copy link
Contributor

Choose a reason for hiding this comment

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

How about grouping the pubkey properties?

e.g.

{
  "address": ...,
  "sequence": ...,
  "pub_key": {
    "type":...,
    "key":...,
  }
}

Copy link
Author

Choose a reason for hiding this comment

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

Okay, I apply it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enable test-race
3 participants