Refactor x/staking Validation and Delegation tests based on MsgCreateValidator.Pubkey type change. #7526

robert-zaremba · 2020-10-13T18:59:59Z

This Changeset introduces set of improvements for writing tests based on
MsgCreateValidator.Pubkey type change from string to Any

The changes here are driven by the tests updates related to message creation
in x/staking.

Motivation

Lot of tests in the SDK don't have any abstraction. This makes tests hard to maintain. Common symptoms are:

lot of code repetition.
long tests (by the amount of lines)

As presented in this case, a small change in the code (type change and introducing one return error) can lead to many changes in the tests.

Solution

The idea is to create a testing subpackage which will provide functions
to make tests more dev-friendly and wrap higher level use-cases.
Here is a show-up of of creating a testing service for staking module. That service encapsulates common logic, handles the error checks.

Related to

Improve testing DevX Improve testing DevX #7362
Remove bech32 PubKey support Remove bech32 PubKey support #7477

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

amaury1093

I love PRs that have a negative line diff, so 👍 for now. But a lot of tests still need to be fixed, will review again once that's the case.

proto/cosmos/staking/v1beta1/tx.proto

testutil/network/network.go

x/staking/teststaking/service.go

x/slashing/keeper/test_common.go

aaronc · 2020-10-14T13:45:44Z

codec/types/any.go

+// PackAny is a checked and safe version of UnsafePackAny. It assures that
+// `x` implements the proto.Message interface and uses it to serialize `x`.
+// TODO: should be moved away: https://github.com/cosmos/cosmos-sdk/issues/7479
+func PackAny(x interface{}) (*Any, error) {


Suggested change

func PackAny(x interface{}) (*Any, error) {

func LegacyPackAny(x interface{}) (*Any, error) {

The name should clearly indicate that this is only for legacy code. I would even add a deprecated comment.

I've market this function as deprecated.

robert-zaremba · 2020-10-14T14:53:35Z

I must say that I'm using that the test-build-check make job is useful in this task. Would be good to revisit it. #7364

This Changeset introduces set of improvements for writing tests. The idea is to create a testing subpackage which will provide functions to make tests more dev-friendly and wrap higher level use-cases. Here is a show-up of of creating a service for staking module for tests. This PR also changes the `x/staking/types.MsgCreateValidator.Pubkey` from string to types.Any. This change motivated the other change to show the pattern I'm describing here.

blushi

You need to implement UnpackInterfaces method on MsgCreateValidator since it now contains an Any.

EDIT: done in 11924dd

x/staking/types/msg_test.go

…rt/staking-test-service

codecov · 2020-10-16T16:21:04Z

Codecov Report

Merging #7526 into master will decrease coverage by 0.00%.
The diff coverage is 41.66%.

@@            Coverage Diff             @@
##           master    #7526      +/-   ##
==========================================
- Coverage   54.15%   54.15%   -0.01%     
==========================================
  Files         606      606              
  Lines       38164    38187      +23     
==========================================
+ Hits        20669    20680      +11     
- Misses      15399    15410      +11     
- Partials     2096     2097       +1

clevinson · 2020-10-17T00:33:00Z

There was one remaining failing test which I just fixed, so I think all tests should be passing now. I left out migrating Validator.ConsensusPubKey, as I think this PR is mostly focused on the testing improvements.

I added this to the description of #7477 so we don't forget.

types/address.go

robert-zaremba · 2020-10-17T08:14:36Z

Thank you for helping with fixing tests.

robert-zaremba · 2020-10-17T08:50:59Z

There are few other things failing, but the crux is solved (Protobuf decoding) 💪
I will be tomorrow with a computer and can handle the remaining tests.

robert-zaremba · 2020-10-18T22:11:15Z

I fixed the remaining 2 tests and renamed Service to Handler.
IMHO the PR is ready to be merged.

amaury1093

utACK

amaury1093 · 2020-10-19T08:48:34Z

x/staking/types/msg_test.go

+	// description := Description{}
+	// commission1 := NewCommissionRates(sdk.ZeroDec(), sdk.ZeroDec(), sdk.ZeroDec())
+	// msg, err := NewMsgCreateValidator(valAddr1, pk1, coinPos, description, commission1, sdk.OneInt())
+	// require.NoError(t, err)


let's not leave this uncommented

Ah, thanks for spotting.

x/staking/teststaking/service.go

amaury1093 · 2020-10-19T08:55:36Z

types/address.go

+		}
+
+	default:
+		// We only allow ed25519 pubkeys to be bech32-ed right now.


Suggested change

// We only allow ed25519 pubkeys to be bech32-ed right now.

I'm removing this in other PR.

addressed

blushi

Looks good to me overall, although I believe new testslashing and teststaking folders names are a bit redundant given that they're already in x/slashing and x/staking resp. Why not using a more concise name?

robert-zaremba · 2020-10-19T12:40:10Z

Looks good to me overall, although I believe new testslashing and teststaking folders names are a bit redundant given that they're already in x/slashing and x/staking resp. Why not using a more concise name?

I was already explaning that in few places. The problem is that we want to use very generic names (helper, test, common.... 😦 ) but we put there context specific code (eg, things related to given module, or codec). This causes name clashes and abuses package aliases.

If I rename it to testing (alessio made that suggestion) then it will conflict with the std lib, which we MUST NOT do.
if I rename to helper / common etc... it will conflict with other types and we end up doing aforementioned alisases as we do with codectypes, sdk, authtypes etc....
moreover, this confuses the editor, and makes the automatic import not working, eg: helper.NewHelper can refer to any module helper package.

TL;DR: cointinuing this practice is rather diminishing the quality of code, DevX and tooling support.
and with aliases, the name won't be more concise 😉

…Validator.Pubkey type change. (#7526) * testing: refactore Validation and Delegation handling of x/staking This Changeset introduces set of improvements for writing tests. The idea is to create a testing subpackage which will provide functions to make tests more dev-friendly and wrap higher level use-cases. Here is a show-up of of creating a service for staking module for tests. This PR also changes the `x/staking/types.MsgCreateValidator.Pubkey` from string to types.Any. This change motivated the other change to show the pattern I'm describing here. * add validator checks * type change fixes * use deprecated * adding test slashing * new network comment update * working on tests * Fix TestMsgPkDecode test * Add UnpackInterfaces to MsgCreateValidator * Fix tests * Convert bech32 pubkey to proto * Fix test * fix v039/migrate_test/TestMigrate * fix tests * testslashing: rename Service to Helper * file rename * update TestMsgDecode Co-authored-by: blushi <marie.gauthier63@gmail.com> Co-authored-by: Amaury Martiny <amaury.martiny@protonmail.com> Co-authored-by: Cory Levinson <cjlevinson@gmail.com>

…Validator.Pubkey type change. (cosmos#7526) * testing: refactore Validation and Delegation handling of x/staking This Changeset introduces set of improvements for writing tests. The idea is to create a testing subpackage which will provide functions to make tests more dev-friendly and wrap higher level use-cases. Here is a show-up of of creating a service for staking module for tests. This PR also changes the `x/staking/types.MsgCreateValidator.Pubkey` from string to types.Any. This change motivated the other change to show the pattern I'm describing here. * add validator checks * type change fixes * use deprecated * adding test slashing * new network comment update * working on tests * Fix TestMsgPkDecode test * Add UnpackInterfaces to MsgCreateValidator * Fix tests * Convert bech32 pubkey to proto * Fix test * fix v039/migrate_test/TestMigrate * fix tests * testslashing: rename Service to Helper * file rename * update TestMsgDecode Co-authored-by: blushi <marie.gauthier63@gmail.com> Co-authored-by: Amaury Martiny <amaury.martiny@protonmail.com> Co-authored-by: Cory Levinson <cjlevinson@gmail.com>

robert-zaremba requested review from aaronc, alessio and alexanderbez as code owners October 13, 2020 18:59

robert-zaremba requested review from amaury1093, blushi, anilcse and atheeshp and removed request for alessio October 13, 2020 19:06

robert-zaremba force-pushed the robert/staking-test-service branch from a5978f0 to e7dca72 Compare October 14, 2020 11:13

amaury1093 added the backport/0.40.x (Stargate) label Oct 14, 2020

amaury1093 reviewed Oct 14, 2020

View reviewed changes

proto/cosmos/staking/v1beta1/tx.proto Show resolved Hide resolved

testutil/network/network.go Outdated Show resolved Hide resolved

x/staking/teststaking/service.go Outdated Show resolved Hide resolved

x/slashing/keeper/test_common.go Outdated Show resolved Hide resolved

aaronc previously requested changes Oct 14, 2020

View reviewed changes

aaronc changed the title ~~Refactore x/staking Validation and Delegation tests based on MsgCreateValidator.Pubkey type change.~~ Refactor x/staking Validation and Delegation tests based on MsgCreateValidator.Pubkey type change. Oct 14, 2020

aaronc added this to the v0.40 [Stargate] milestone Oct 14, 2020

robert-zaremba added 7 commits October 15, 2020 23:03

add validator checks

744e299

type change fixes

1741fe0

use deprecated

a29648d

adding test slashing

0090ab3

new network comment update

d527c77

working on tests

1a29257

robert-zaremba force-pushed the robert/staking-test-service branch from 312d579 to 1a29257 Compare October 16, 2020 00:32

blushi suggested changes Oct 16, 2020

View reviewed changes

x/staking/types/msg_test.go Show resolved Hide resolved

x/staking/types/msg_test.go Outdated Show resolved Hide resolved

blushi and others added 4 commits October 16, 2020 16:58

Fix TestMsgPkDecode test

819c52e

Add UnpackInterfaces to MsgCreateValidator

11924dd

Merge branch 'master' of ssh://github.com/cosmos/cosmos-sdk into robe…

e36a543

…rt/staking-test-service

Fix tests

883ced6

Convert bech32 pubkey to proto

9b7d36e

fix v039/migrate_test/TestMigrate

9f8b828

clevinson reviewed Oct 17, 2020

View reviewed changes

types/address.go Show resolved Hide resolved

robert-zaremba added 2 commits October 19, 2020 00:03

fix tests

b610de8

testslashing: rename Service to Helper

16448b8

robert-zaremba requested review from amaury1093, clevinson, blushi and aaronc October 18, 2020 22:11

amaury1093 approved these changes Oct 19, 2020

View reviewed changes

robert-zaremba added the A:automerge Automatically merge PR once all prerequisites pass. label Oct 19, 2020

file rename

b741a6d

robert-zaremba removed the A:automerge Automatically merge PR once all prerequisites pass. label Oct 19, 2020

update TestMsgDecode

b7eef7d

blushi reviewed Oct 19, 2020

View reviewed changes

blushi self-requested a review October 19, 2020 12:41

blushi approved these changes Oct 19, 2020

View reviewed changes

Merge branch 'master' into robert/staking-test-service

826e771

robert-zaremba added the A:automerge Automatically merge PR once all prerequisites pass. label Oct 19, 2020

mergify bot merged commit 8eaf2ec into master Oct 19, 2020

mergify bot deleted the robert/staking-test-service branch October 19, 2020 13:04

robert-zaremba mentioned this pull request Oct 20, 2020

Remove bech32 PubKey support #7447

Closed

9 tasks

clevinson mentioned this pull request Oct 29, 2020

[backports]: v0.40.0-rc2 Cherry-picks #7734

Closed

45 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor x/staking Validation and Delegation tests based on MsgCreateValidator.Pubkey type change. #7526

Refactor x/staking Validation and Delegation tests based on MsgCreateValidator.Pubkey type change. #7526

robert-zaremba commented Oct 13, 2020 •

edited

Loading

amaury1093 left a comment

aaronc Oct 14, 2020

robert-zaremba Oct 18, 2020

robert-zaremba commented Oct 14, 2020

blushi left a comment •

edited

Loading

codecov bot commented Oct 16, 2020 •

edited

Loading

clevinson commented Oct 17, 2020

robert-zaremba commented Oct 17, 2020

robert-zaremba commented Oct 17, 2020

robert-zaremba commented Oct 18, 2020

amaury1093 left a comment

amaury1093 Oct 19, 2020

robert-zaremba Oct 19, 2020

amaury1093 Oct 19, 2020

robert-zaremba Oct 19, 2020

blushi left a comment •

edited

Loading

robert-zaremba commented Oct 19, 2020

	func PackAny(x interface{}) (*Any, error) {
	func LegacyPackAny(x interface{}) (*Any, error) {

Refactor x/staking Validation and Delegation tests based on MsgCreateValidator.Pubkey type change. #7526

Refactor x/staking Validation and Delegation tests based on MsgCreateValidator.Pubkey type change. #7526

Conversation

robert-zaremba commented Oct 13, 2020 • edited Loading

Motivation

Solution

Related to

amaury1093 left a comment

Choose a reason for hiding this comment

aaronc Oct 14, 2020

Choose a reason for hiding this comment

robert-zaremba Oct 18, 2020

Choose a reason for hiding this comment

robert-zaremba commented Oct 14, 2020

blushi left a comment • edited Loading

Choose a reason for hiding this comment

codecov bot commented Oct 16, 2020 • edited Loading

Codecov Report

clevinson commented Oct 17, 2020

robert-zaremba commented Oct 17, 2020

robert-zaremba commented Oct 17, 2020

robert-zaremba commented Oct 18, 2020

amaury1093 left a comment

Choose a reason for hiding this comment

amaury1093 Oct 19, 2020

Choose a reason for hiding this comment

robert-zaremba Oct 19, 2020

Choose a reason for hiding this comment

amaury1093 Oct 19, 2020

Choose a reason for hiding this comment

robert-zaremba Oct 19, 2020

Choose a reason for hiding this comment

blushi left a comment • edited Loading

Choose a reason for hiding this comment

robert-zaremba commented Oct 19, 2020

robert-zaremba commented Oct 13, 2020 •

edited

Loading

blushi left a comment •

edited

Loading

codecov bot commented Oct 16, 2020 •

edited

Loading

blushi left a comment •

edited

Loading