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 setup for cli_test #6033

Merged
merged 61 commits into from
Apr 29, 2020
Merged

Conversation

sahith-narahari
Copy link
Contributor

@sahith-narahari sahith-narahari commented Apr 20, 2020

ref: #5951

Description


For contributor use:

  • 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

For admin use:

  • Added appropriate labels to PR (ex. WIP, R4R, docs, etc)
  • Reviewers assigned
  • Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

@sahith-narahari sahith-narahari changed the title Sahith/add base setup Add setup for cli_test Apr 20, 2020
Copy link
Member

@aaronc aaronc left a comment

Choose a reason for hiding this comment

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

This looks like a good start. Can you add a github actions CI task to run test-cli @sahith-narahari ? I think we need to modify test-cover.sh so that these tests don't run there and so that they run in a separate CI task.

@sahith-narahari
Copy link
Contributor Author

sahith-narahari commented Apr 20, 2020

thanks @aaronc , added test-cli to circleci

@aaronc
Copy link
Member

aaronc commented Apr 20, 2020

thanks @aaronc , added test-cli to circleci

@sahith-narahari, sorry not circleci, github actions please. My understanding is that we're trying to migrate all CI there.

@aaronc
Copy link
Member

aaronc commented Apr 27, 2020

IMHO cli_test/helpers functions and types should live inside our top-level tests package. That's where we've placed all testing-related auxiliary functions and types so far. I would even avoid creating a tests/cli subdir - dropping everything in cli_helpers.go would possibly just do it.

I am in favor of moving cli_test -> tests/cli so that all test stuff lives under tests. I would caution against putting everything in a cli_helpers.go file. I think the reorganization that @sahith-narahari is helpful and we shouldn't move back to a single large file with everything.

So I would say let's create two package there - tests/cli/helpers and tests/cli/tests instead of a new cli_test root package.

Are you okay with that @alessio ?

@alessio
Copy link
Contributor

alessio commented Apr 27, 2020

sounds good to me @aaronc

@aaronc
Copy link
Member

aaronc commented Apr 28, 2020

What is the plan to get this to completion? Will each module be done in a separate PR (this is what I recommend)?

Yes the plan is to address this module by module. I believe some of that work has already started.

@alexanderbez
Copy link
Contributor

Great! What is left remaining for this PR to be wrapped up?

@codecov
Copy link

codecov bot commented Apr 28, 2020

Codecov Report

Merging #6033 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #6033   +/-   ##
=======================================
  Coverage   54.78%   54.78%           
=======================================
  Files         437      437           
  Lines       26433    26433           
=======================================
  Hits        14482    14482           
  Misses      10955    10955           
  Partials      996      996           

@aaronc
Copy link
Member

aaronc commented Apr 28, 2020

Great! What is left remaining for this PR to be wrapped up?

Looks like just some fixes to CI. Once that's fixed LGTM

Copy link
Member

@aaronc aaronc left a comment

Choose a reason for hiding this comment

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

LGTM 🎉


return gs.Validate()
// TODO: UNDO this when DefaultGenesis() is implemented
Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Member

@aaronc aaronc Apr 29, 2020

Choose a reason for hiding this comment

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

As @anilcse has noted here, there is an issue with IBC ValidateGenesis that was breaking cli tests. I suggest we merge this PR with ValidateGenesis commented out like this so that cli tests can proceed. When the IBC team fixes this issue, they can uncomment these lines.

Copy link
Contributor

Choose a reason for hiding this comment

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

ACK

Copy link
Contributor

@clevinson clevinson May 4, 2020

Choose a reason for hiding this comment

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

@anilcse Is there a currently open issue / PR for tracking this and making sure this doesn't get forgotten? #5948 seems to be closed, and I cant find any open related issues.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Threre's none. May be we can reopen this issue: #6082

Copy link
Collaborator

Choose a reason for hiding this comment

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

there is an issue with IBC ValidateGenesis that was breaking cli tests.

which one?

Copy link
Collaborator

Choose a reason for hiding this comment

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

DefaultGenesis is not implemented and it just returns nil

DefaultGenesis is defined in x/ibc/genesis.go

Copy link
Contributor

@alexanderbez alexanderbez left a comment

Choose a reason for hiding this comment

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

ACK

@alexanderbez alexanderbez dismissed fedekunze’s stale review April 29, 2020 14:02

Changed requested addressed and/or planned to be in subsequent PRs.

@alexanderbez alexanderbez merged commit 3b71198 into cosmos:master Apr 29, 2020
@clevinson clevinson added this to the v0.39 milestone May 4, 2020
@sahith-narahari sahith-narahari deleted the sahith/add-base-setup branch May 15, 2020 12:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants