Cleanup Baseapp tests #1579

ebuchman · 2018-07-06T16:17:36Z

Working on #1556

Will also likely address #1120 and replace some of #1172

Updated all relevant documentation in docs
Updated all code comments where relevant
Wrote tests
Updated CHANGELOG.md
Updated Gaia/Examples
Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)
Added appropriate labels to PR (ex. wip, ready-for-review, docs)

codecov · 2018-07-06T16:29:03Z

Codecov Report

Merging #1579 into develop will increase coverage by 0.34%.
The diff coverage is 85.71%.

@@             Coverage Diff             @@
##           develop    #1579      +/-   ##
===========================================
+ Coverage    64.18%   64.53%   +0.34%     
===========================================
  Files          122      122              
  Lines         6674     6682       +8     
===========================================
+ Hits          4284     4312      +28     
+ Misses        2145     2125      -20     
  Partials       245      245

ebuchman · 2018-07-07T17:12:43Z

Ok, this is ready aside from the changes to not run msg handlers on check tx.

It involved some fixes to the baseapp.

ebuchman · 2018-07-07T17:15:36Z

Should I rebase/squash?

ValarDragon · 2018-07-07T17:16:54Z

baseapp/router.go

@@ -36,7 +36,7 @@ var isAlpha = regexp.MustCompile(`^[a-zA-Z]+$`).MatchString
 // AddRoute - TODO add description
 func (rtr *router) AddRoute(r string, h sdk.Handler) Router {
 	if !isAlpha(r) {
-		panic("route expressions can only contain alphanumeric characters")
+		panic("route expressions can only contain alphabet characters")


Good catch! However I think the regex not allowing numerics is the bug here, not the error message being mismatched. I think there are conceivable reasons for wanting numbers in routers.

Let's open a new issue for this

ValarDragon · 2018-07-07T17:21:28Z

baseapp/baseapp_test.go

@@ -270,12 +290,10 @@ type testTx struct {

 const msgType2 = "testTx"


Could we add a comment here saying that the point of this is that its a sample message that can fail ValidateBasic?

ValarDragon · 2018-07-07T17:25:27Z

baseapp/baseapp_test.go

+		return
+	})
+
+	tx := testTx{13}


I found the 13 here confusing. (Took me a bit to remember where it came from) Maybe we should change testTx from taking in a number to just taking in a bool for whether or not to fail.

ebuchman · 2018-07-07T17:30:10Z

@ValarDragon thanks for initial review. I rebased on develop and squashed it down to 2 commits. I think some of your comments were addressed in later commits

ebuchman · 2018-07-07T17:31:16Z

It should probably just be one commit

ebuchman · 2018-07-07T17:35:00Z

Ok squash down to one commit.

Let's get this merged - @cwgoes could use review on the changes to runTx for handling out-of-gas.

I'll work on #1120 in a separate PR.

ValarDragon · 2018-07-07T17:38:14Z

baseapp/baseapp.go

@@ -534,53 +538,74 @@ func (app *BaseApp) runTx(mode runTxMode, txBytes []byte, tx sdk.Tx) (result sdk
 		ctx = ctx.WithMultiStore(msCache)
 	}

-	finalResult := sdk.Result{}
+	// accumulate results
 	var logs []string


Can you do make([]string, 0, len(msgs), saves overhead due to expanding the slice.

* simplify mock tx type, msgs, and handlers * remove dependencies on auth and bank * dedup with setupBaseApp * lots of comments and cleanup * fixes where we weren't checking results * use some table driven tests * remove TestValidatorChange - its not testing anything since baseapp doesnt track validator changes * prepare for CheckTx only running the AnteHandler * fix runTx gas handling and add more tests * new tests for multi-msgs

ValarDragon · 2018-07-07T17:43:50Z

baseapp/baseapp.go

+		ctx = app.checkState.ctx.WithTxBytes(txBytes)
+	} else {
+		ctx = app.deliverState.ctx.WithTxBytes(txBytes)
+		ctx = ctx.WithSigningValidators(app.signedValidators)


Maybe I'm unclear on what the purpose of simulate is, but why doesn't it get access to any of the app's signed validators? (How can it simulate how a slashing or staking tx will happen w/o this?)

Yeh good question. The problem is that information is only supplied by BeginBlock, and when we're simulating, we don't know what the next BeginBlock will look like ...

Hrmm. Would it make sense to have an option where its simulated as the last tx in the last known block (for things that depend on headers / validator set), and an option for on the next block (for conceivable things that depend on the exact block height)? This discussion should probably be carried out in a separate issue though

Could be useful, although that seems like a rare case - all slashing logic related to signing (downtime / evidence) happens in a BeginBlock handler, not in transactions.

cwgoes

Baseapp changes LGTM, a few future suggestions but nothing that needs to block merge.

cwgoes · 2018-07-07T19:46:52Z

baseapp/baseapp.go

+	// accumulate results
+	logs := make([]string, 0, len(msgs))
+	var data []byte   // NOTE: we just append them all (?!)
+	var tags sdk.Tags // also just append them all


tendermint/tendermint#1803 will provide a better option here

cwgoes · 2018-07-07T19:49:35Z

baseapp/baseapp.go

 		}
 		if !newCtx.IsZero() {
 			ctx = newCtx
 		}
+		gasWanted = anteResult.GasWanted


Presently the ante handler returns an empty sdk.Result{} - did we want it to set GasWanted (to tx.Fee.Gas presumably)?

cwgoes · 2018-07-07T19:53:21Z

baseapp/baseapp.go

+		ctx = app.checkState.ctx.WithTxBytes(txBytes)
+	} else {
+		ctx = app.deliverState.ctx.WithTxBytes(txBytes)
+		ctx = ctx.WithSigningValidators(app.signedValidators)


Could be useful, although that seems like a rare case - all slashing logic related to signing (downtime / evidence) happens in a BeginBlock handler, not in transactions.

cwgoes · 2018-07-07T19:54:57Z

baseapp/baseapp.go

-	for i, msg := range msgs {
+	// accumulate results
+	logs := make([]string, 0, len(msgs))
+	var data []byte   // NOTE: we just append them all (?!)


I'm still not clear what exactly data and logs are for, and what the difference between them is intended to be.

At the moment, I think the SDK uses data in one instance to return a proposal ID (in governance), and doesn't use logs at all. The created proposal ID could just as easily be returned as a tag (probably will be anyways, for later querying) - maybe we can simplify ABCI or what parts of ABCI we use?

log is intended for non-deterministic output from the app. There is some sense that it would be used to pipe the apps actual logs back to tendermint but I don't think anyone has ever done that.

data is a deterministic result - we don't really use it AFAIK, so it probably could just be absorbed into tags, but it might be worth keeping for simple apps that want a definitive return value

ebuchman requested review from cwgoes and rigelrozanski as code owners July 6, 2018 16:17

ebuchman changed the title ~~WIP: Cleanup Baseapp tests~~ Cleanup Baseapp tests Jul 7, 2018

ValarDragon reviewed Jul 7, 2018

View reviewed changes

ebuchman force-pushed the bucky/baseapp-tests branch 2 times, most recently from fb3e7b1 to 31a1db9 Compare July 7, 2018 17:28

ebuchman force-pushed the bucky/baseapp-tests branch from 31a1db9 to fe7ae11 Compare July 7, 2018 17:32

ValarDragon reviewed Jul 7, 2018

View reviewed changes

ValarDragon mentioned this pull request Jul 7, 2018

baseapp: Should routes allow numbers #1587

Closed

ebuchman mentioned this pull request Jul 7, 2018

Tx interface #670

Closed

ebuchman added the ready-for-review label Jul 7, 2018

This was referenced Jul 7, 2018

Fix tx gas used calculation #1585

Closed

baseapp: simplify tests #1556

Closed

x: can we improve testing by reducing dependence on mock ? #1555

Closed

WIP: Baseapp to not run messages/msgHandler on CheckTx #1172

Closed

fixes from review

6159554

cwgoes approved these changes Jul 7, 2018

View reviewed changes

ebuchman merged commit 988614e into develop Jul 7, 2018

ebuchman deleted the bucky/baseapp-tests branch July 7, 2018 23:04

ebuchman mentioned this pull request Jul 7, 2018

x/auth: follow up from baseapp test cleanup #1592

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cleanup Baseapp tests #1579

Cleanup Baseapp tests #1579

ebuchman commented Jul 6, 2018 •

edited

Loading

codecov bot commented Jul 6, 2018 •

edited

Loading

ebuchman commented Jul 7, 2018 •

edited

Loading

ebuchman commented Jul 7, 2018

ValarDragon Jul 7, 2018 •

edited

Loading

ebuchman Jul 7, 2018

ValarDragon Jul 7, 2018

ValarDragon Jul 7, 2018

ebuchman commented Jul 7, 2018

ebuchman commented Jul 7, 2018

ebuchman commented Jul 7, 2018

ValarDragon Jul 7, 2018 •

edited

Loading

ValarDragon Jul 7, 2018

ebuchman Jul 7, 2018

ValarDragon Jul 7, 2018

cwgoes Jul 7, 2018

cwgoes left a comment

cwgoes Jul 7, 2018

cwgoes Jul 7, 2018

ebuchman Jul 7, 2018

cwgoes Jul 7, 2018

cwgoes Jul 7, 2018

ebuchman Jul 7, 2018

		@@ -270,12 +290,10 @@ type testTx struct {

		const msgType2 = "testTx"

Cleanup Baseapp tests #1579

Cleanup Baseapp tests #1579

Conversation

ebuchman commented Jul 6, 2018 • edited Loading

codecov bot commented Jul 6, 2018 • edited Loading

Codecov Report

ebuchman commented Jul 7, 2018 • edited Loading

ebuchman commented Jul 7, 2018

ValarDragon Jul 7, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ebuchman commented Jul 7, 2018

ebuchman commented Jul 7, 2018

ebuchman commented Jul 7, 2018

ValarDragon Jul 7, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cwgoes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ebuchman commented Jul 6, 2018 •

edited

Loading

codecov bot commented Jul 6, 2018 •

edited

Loading

ebuchman commented Jul 7, 2018 •

edited

Loading

ValarDragon Jul 7, 2018 •

edited

Loading

ValarDragon Jul 7, 2018 •

edited

Loading