Cover transaction manager with additional test cases #561 #601

canercidam · 2018-02-01T21:03:22Z

Changes setupTransactionPoolAPI to prepare mock functions in a more strictly covering way, regarding gas, gas price and resulting transaction.

Closes #561

@dshulyak

canercidam · 2018-02-01T21:16:28Z

I'm going to extend TestCompleteTransaction in a table-driven manner with non-nil gas and gas price cases, if these changes look good enough.

dshulyak · 2018-02-02T07:50:06Z

Yes, this approach looks good. #537 was merged, so now it is possible to overwrite timeout for waiting.

But what about that wg.Wait? Did you found what was causing that problem?

canercidam · 2018-02-02T12:57:29Z

ctx, cancel = context.WithTimeout(context.Background(), defaultTimeout)
defer cancel()
if err := m.ethTxClient.SendTransaction(ctx, signedTx); err != nil {
	return hash, err
}

Looks like defaultTimeout affects that, so I'm trying to see in source code how context timeout keeps

return ec.c.CallContext(ctx, nil, "eth_sendRawTransaction", common.ToHex(data))

line in SendTransaction from finalizing. I think the quick solution at the moment is having a ctxTimeout field in Manager as well and setting it to a low value in tests.

canercidam · 2018-02-02T13:36:24Z

go-ethereum/rpc/initproc.go, L29:

go handler.ServeCodec(NewJSONCodec(p1), OptionMethodInvocation|OptionSubscriptions)

I believe that goroutine is getting killed on the server side so client context is still alive until it times out.

canercidam · 2018-02-02T15:37:29Z

It's also possible to pull TestAccountMismatch and TestInvalidPassword into TestCompleteTransaction now.

dshulyak · 2018-02-02T16:24:51Z

@canercidam thanks for a research, i think you are right. adding default timeout that is used for context to transaction manager and then overwriting it in tests sounds goods. i think that we should even use transaction context for every call, but it is definitely not for this PR.

i will do a review tomorrow probably

dshulyak

overall looks good, need to verify how it works with testify and added a couple of style comments

dshulyak · 2018-02-05T06:44:26Z

geth/transactions/txqueue_manager.go

@@ -32,6 +32,7 @@ type Manager struct {
 	addrLock          *AddrLocker
 	notify            bool
 	completionTimeout time.Duration
+	ctxTimeout        time.Duration


rpcCallTimeout ?

dshulyak · 2018-02-05T06:45:22Z

geth/transactions/txqueue_manager_test.go

-	s.txServiceMock.EXPECT().GasPrice(gomock.Any()).Return(big.NewInt(10), nil)
-	s.txServiceMock.EXPECT().EstimateGas(gomock.Any(), gomock.Any()).Return(&gas, nil)
-	s.txServiceMock.EXPECT().SendRawTransaction(gomock.Any(), gomock.Any()).Return(gethcommon.Hash{}, txErr)
+func (s *TxQueueTestSuite) setupTransactionPoolAPI(tx *common.QueuedTx, config *params.NodeConfig, account *common.SelectedExtKey, nonce *hexutil.Uint64, gas *hexutil.Big, txErr error) {


what do you think about preparing tx earlier and passing it to this method? it looks like it will cleanup an interface

Hmm, what do you mean by "preparing tx earlier and passing it to this method"? Isn't that already how it works?

i thought that it is possible to pass types.Transaction instead of tx *common.QueuedTx, config *params.NodeConfig, account *common.SelectedExtKey, nonce *hexutil.Uint64, gas *hexutil.Big

I see what you mean, I may be able to further reduce the code using types.Transaction. I'll look into it.

On a second glance, it will just result in moving lines elsewhere because one has to form both types.Transaction and common.QueuedTx objects in the same case. In other words, if I define a types.Transaction object in the test case and pass it to setupTransactionPoolAPI instead, that means I can remove forming a new object from s.rlpEncode and it doesn't make a difference about how I handle rest of the args such as gas, gas price and nonce.

But it's definitely possible to reduce the amount of args there by having default test values so I'm going to do that instead.

ok, thanks for checking

dshulyak · 2018-02-05T06:46:38Z

geth/transactions/txqueue_manager_test.go

-	s.NoError(WaitClosed(w, time.Second))
-	s.Equal(hash, rst.Hash)
+	for _, testCase := range testCases {
+		s.T().Run(testCase.name, func(t *testing.T) {


never used it with testify, will it allow to use testify.m the same way as before?

Looks like it doesn't detect that. But IMHO, it's certainly more readable & DRY and all tests run in few seconds anyway.

looks good i think

go test ./geth/transactions/ -v -testify.m=TestCompleteTransaction$
=== RUN TestTxQueueTestSuite
=== RUN TestTxQueueTestSuite/TestCompleteTransaction
=== RUN TestTxQueueTestSuite/TestCompleteTransaction/noGasDef
=== RUN TestTxQueueTestSuite/TestCompleteTransaction/gasDefined
=== RUN TestTxQueueTestSuite/TestCompleteTransaction/gasPriceDefined
--- PASS: TestTxQueueTestSuite (0.01s)
--- PASS: TestTxQueueTestSuite/TestCompleteTransaction (0.01s)
--- PASS: TestTxQueueTestSuite/TestCompleteTransaction/noGasDef (0.00s)
--- PASS: TestTxQueueTestSuite/TestCompleteTransaction/gasDefined (0.00s)
--- PASS: TestTxQueueTestSuite/TestCompleteTransaction/gasPriceDefined (0.00s)
PASS
ok github.com/status-im/status-go/geth/transactions 0.055s

dshulyak

Needs to be rebased ofcourse

canercidam · 2018-02-05T14:30:12Z

@dshulyak Can you please review the last small change?

dshulyak · 2018-02-05T14:42:15Z

geth/transactions/txqueue_manager_test.go

+var (
+	testGas      = (*hexutil.Big)(big.NewInt(defaultGas + 1))
+	testGasPrice = (*hexutil.Big)(big.NewInt(10))
+	testNonce    = hexutil.Uint64(10)


i will be using non-default nonce here #538 , so it is not very good idea. but you can leave it if you want. i can rebase

Oh, okay. I'll just revert changes for nonce then.

mandrigin

Looks good 👍

canercidam · 2018-02-05T16:41:43Z

@mandrigin Whoops, it wasn't ready yet. :)

canercidam · 2018-02-05T16:43:00Z

Anyway, doesn't make a big difference. I was going to rollback a change and squash commits. Thanks for your time and efforts! @mandrigin @dshulyak

canercidam added 4 commits February 1, 2018 03:43

more configuration for setupTransactionPoolAPI

d81554b

change timeout

4c3d4be

fix bad expected values

155a424

revert timeout

35a2a31

merge fixes from status-im#537

e3d708a

canercidam added 2 commits February 2, 2018 16:45

fix hanging test

16a05eb

table-driven gas and gas price tests

1b3dbde

dshulyak suggested changes Feb 5, 2018

View reviewed changes

rename ctxTimeout to rpcCallTimeout

70e00f7

dshulyak approved these changes Feb 5, 2018

View reviewed changes

reduce args

347f726

dshulyak approved these changes Feb 5, 2018

View reviewed changes

mandrigin approved these changes Feb 5, 2018

View reviewed changes

mandrigin merged commit 0dd47ab into status-im:develop Feb 5, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cover transaction manager with additional test cases #561 #601

Cover transaction manager with additional test cases #561 #601

canercidam commented Feb 1, 2018

canercidam commented Feb 1, 2018

dshulyak commented Feb 2, 2018

canercidam commented Feb 2, 2018 •

edited

Loading

canercidam commented Feb 2, 2018 •

edited

Loading

canercidam commented Feb 2, 2018

dshulyak commented Feb 2, 2018

dshulyak left a comment

dshulyak Feb 5, 2018

dshulyak Feb 5, 2018

canercidam Feb 5, 2018

dshulyak Feb 5, 2018

canercidam Feb 5, 2018

canercidam Feb 5, 2018 •

edited

Loading

dshulyak Feb 5, 2018

dshulyak Feb 5, 2018

canercidam Feb 5, 2018

dshulyak Feb 5, 2018

dshulyak left a comment

canercidam commented Feb 5, 2018

dshulyak Feb 5, 2018

canercidam Feb 5, 2018

mandrigin left a comment

canercidam commented Feb 5, 2018

canercidam commented Feb 5, 2018

Cover transaction manager with additional test cases #561 #601

Cover transaction manager with additional test cases #561 #601

Conversation

canercidam commented Feb 1, 2018

canercidam commented Feb 1, 2018

dshulyak commented Feb 2, 2018

canercidam commented Feb 2, 2018 • edited Loading

canercidam commented Feb 2, 2018 • edited Loading

canercidam commented Feb 2, 2018

dshulyak commented Feb 2, 2018

dshulyak 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

canercidam Feb 5, 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

dshulyak left a comment

Choose a reason for hiding this comment

canercidam commented Feb 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mandrigin left a comment

Choose a reason for hiding this comment

canercidam commented Feb 5, 2018

canercidam commented Feb 5, 2018

canercidam commented Feb 2, 2018 •

edited

Loading

canercidam commented Feb 2, 2018 •

edited

Loading

canercidam Feb 5, 2018 •

edited

Loading