core: migrate native contract API tests to neotest framework #2299

AnnaShaleva · 2021-12-03T16:01:15Z

Part of #1472.

Migration goes well, but some tests need to be adjusted, so test refactoring is being made along the way. Progress:

pkg/nativetest/common_test.go

roman-khimov · 2021-12-06T12:46:07Z

pkg/core/native_policy_test.go

 }

 func TestBlockedAccounts(t *testing.T) {
 	chain := newTestChain(t)
-	account := util.Uint160{1, 2, 3}
-	policyHash := chain.contracts.Policy.Metadata().Hash
-
 	transferTokenFromMultisigAccount(t, chain, testchain.CommitteeScriptHash(),
 		chain.contracts.GAS.Hash, 100_00000000)

 	t.Run("isBlocked, internal method", func(t *testing.T) {


These checks can be moved too, I think. We're testing native package here anyway.

We're testing internal method here and it uses dao to retrieve blocked accounts. Our native tests can't be dependant on dao.

AnnaShaleva · 2021-12-09T17:25:51Z

Depends on #2298, still in progress.

codecov · 2021-12-15T11:28:53Z

Codecov Report

Merging #2299 (45f543a) into master (501ca0d) will increase coverage by 0.02%.
The diff coverage is 79.74%.

❗ Current head 45f543a differs from pull request most recent head 0f85b4e. Consider uploading reports for the commit 0f85b4e to get more accurate results

@@            Coverage Diff             @@
##           master    #2299      +/-   ##
==========================================
+ Coverage   84.01%   84.03%   +0.02%     
==========================================
  Files         307      308       +1     
  Lines       29840    29913      +73     
==========================================
+ Hits        25070    25138      +68     
+ Misses       3350     3347       -3     
- Partials     1420     1428       +8

Impacted Files	Coverage Δ
...ve/nativetest/helpers/policyhelper/policyhelper.go	`0.00% <0.00%> (ø)`
pkg/vm/emit/emit.go	`88.88% <22.22%> (-11.12%)`	⬇️
pkg/neotest/basic.go	`92.78% <100.00%> (+5.14%)`	⬆️
pkg/neotest/chain/chain.go	`92.77% <100.00%> (+0.87%)`	⬆️
pkg/neotest/client.go	`100.00% <100.00%> (ø)`
pkg/core/native/oracle.go	`73.85% <0.00%> (-3.27%)`	⬇️
pkg/core/native/management.go	`90.30% <0.00%> (-0.56%)`	⬇️
pkg/vm/stackitem/item.go	`91.63% <0.00%> (-0.38%)`	⬇️
pkg/network/server.go	`73.83% <0.00%> (+0.11%)`	⬆️
pkg/core/native/ledger.go	`94.82% <0.00%> (+0.86%)`	⬆️
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 501ca0d...0f85b4e. Read the comment docs.

AnnaShaleva · 2021-12-15T14:24:36Z

pkg/core/native/native_test/neo_test.go

+			newGAS := e.Chain.GetUtilityTokenBalance(voters[i].ScriptHash())
+			newGAS.Sub(newGAS, gasBalance[i])
+			// TODO: deal with unexported CalculateNEOHolderReward
+			gasForHold, err := e.Chain.CalculateNEOHolderReward(voters[i].ScriptHash(), e.Chain.BlockHeight())


@roman-khimov, I'm not sure how to deal with this part of test. Here we need to compare voter rewards (second part), but CalculateNEOHolderReward is unexported.

Check the value against non-voter (holder) account that holds NEO for the same number of blocks.

Hardcode exact GAS values generated for the tests.

I'm not sure that hardcoded value is good and maintainable, so let's go with the first suggestion. I added these reference accounts and comment on why do we need them. Fixed.

Well, it's a test, it can expect some exact value given known conditions, but comparing to non-voter is more flexible, of course.

AnnaShaleva · 2021-12-15T14:26:32Z

pkg/core/native_designate_test.go

-		bc.getNodesByRole(t, true, noderoles.NeoFSAlphabet, bc.BlockHeight()+1, 1)
-	})
-}
-
 func TestDesignate_DesignateAsRole(t *testing.T) {


This test is aimed to test exported contract methods (not via transaction invocation). However, it can easily be converted to use neotest. So do we need to convert it?

Do we have a counterpart for it that does on-chain invocations? We can have tests like this if for some reason testing them from on-chain call is inappropriate or not possible.

I just tried to convert this tests to on-chain-based, but occurs that we need to check best designated index and it's not possible with on-chain invocations. So I think we'd better leave this test as is.

fyrchik · 2021-12-20T08:04:45Z

pkg/neotest/basic.go

+	h, err := e.Chain.GetNativeContractScriptHash(name)
+	require.NoError(t, err)


Reuse e.NativeHash(t, name) ?

fyrchik · 2021-12-20T08:13:41Z

pkg/core/native/native_test/ledger_test.go

+	for i := 0; i < height-1; i++ {
+		e.AddNewBlock(t)
+	}


Use GenerateNewBlocks from the first commit?

fyrchik · 2021-12-20T08:14:21Z

pkg/core/native/native_test/ledger_test.go

+		for i := 0; i < int(e.Chain.GetConfig().MaxTraceableBlocks); i++ {
+			e.AddNewBlock(t)
+		}


pkg/vm/emit/emit.go

roman-khimov · 2022-01-11T02:09:27Z

pkg/core/native/native_test/neo_test.go

+			newGAS := e.Chain.GetUtilityTokenBalance(voters[i].ScriptHash())
+			newGAS.Sub(newGAS, gasBalance[i])
+			// TODO: deal with unexported CalculateNEOHolderReward
+			gasForHold, err := e.Chain.CalculateNEOHolderReward(voters[i].ScriptHash(), e.Chain.BlockHeight())


Check the value against non-voter (holder) account that holds NEO for the same number of blocks.

Hardcode exact GAS values generated for the tests.

pkg/core/native_neo_test.go

pkg/core/oracle_test.go

pkg/core/native_neo_test.go

AnnaShaleva · 2022-01-13T15:34:32Z

Not yet ready for review, I need to rebase on master and carefully inspect the whole set of changes.

pkg/core/oracle_test.go

Added several methods that are useful for testing.

GAS and NEO tokens are sent to validators account (not the committee's one). For single-node chain they are the same, but for four-nodes chain they are different. Thus, use validators multisig address to create new accounts and to deploy contracts. Also, allow to provide desired account balance while creating new account.

roman-khimov

In general I think this makes tests much more readable, for example notary contract test became shorter. It also proves testing framework viability, which is important. Still, we have some hairy parts also that can be improved in future.

pkg/core/native/native_test/common_test.go

pkg/core/native/native_test/ledger_test.go

pkg/core/native_management_test.go

pkg/core/native/native_test/management_test.go

pkg/core/native/native_test/oracle_test.go

Compiler forces notification parameters to match the one specified in manifest, and manifest of `test_contract.go` tells that `to` parameter type is hash160: ``` event 'Transfer' should have 'Hash160' as type of 1 parameter, got: ByteArray ```

roman-khimov reviewed Dec 6, 2021

View reviewed changes

roman-khimov mentioned this pull request Dec 8, 2021

Store applog with tx #2305

Merged

AnnaShaleva force-pushed the tests-migration branch from 467eb7d to ad60607 Compare December 9, 2021 17:24

roman-khimov mentioned this pull request Dec 9, 2021

Improve signers handling in test framework #2298

Merged

AnnaShaleva force-pushed the tests-migration branch 2 times, most recently from 1378691 to d63828c Compare December 13, 2021 12:31

AnnaShaleva mentioned this pull request Dec 13, 2021

core: fix GAS distribution to primary node when notary enabled #2311

Merged

AnnaShaleva force-pushed the tests-migration branch 7 times, most recently from 5502441 to 45f543a Compare December 15, 2021 11:21

AnnaShaleva force-pushed the tests-migration branch from 45f543a to 0f85b4e Compare December 15, 2021 14:21

AnnaShaleva marked this pull request as ready for review December 15, 2021 14:22

AnnaShaleva requested a review from roman-khimov December 15, 2021 14:23

AnnaShaleva commented Dec 15, 2021

View reviewed changes

fyrchik reviewed Dec 20, 2021

View reviewed changes

fyrchik mentioned this pull request Dec 20, 2021

Way to ignore return result check of invoked method in contract test framework #2312

Closed

AnnaShaleva force-pushed the tests-migration branch from 0f85b4e to 40ad0ab Compare December 27, 2021 10:54

roman-khimov requested changes Jan 11, 2022

View reviewed changes

AnnaShaleva force-pushed the tests-migration branch from 40ad0ab to bd9fcda Compare January 13, 2022 15:31

AnnaShaleva marked this pull request as draft January 13, 2022 15:34

AnnaShaleva commented Jan 13, 2022

View reviewed changes

pkg/core/oracle_test.go Show resolved Hide resolved

neotest: improve neotest API

dcb0616

Added several methods that are useful for testing.

AnnaShaleva and others added 2 commits January 13, 2022 19:12

vm: allow to emit int[32, 16, 8] and some of uints

48355ba

AnnaShaleva force-pushed the tests-migration branch from bd9fcda to 4982a9f Compare January 13, 2022 17:48

AnnaShaleva marked this pull request as ready for review January 13, 2022 17:49

roman-khimov requested changes Jan 13, 2022

View reviewed changes

AnnaShaleva and others added 10 commits January 14, 2022 16:34

nativetest: migrate Policy contract tests to neotest

bbe92c2

nativetest: migrate Ledger contract tests to neotest

4e74350

nativetest: migrate GAS contract tests to neotest framework

3f1af71

nativetest: migrate Designation contract tests to neotest

7927c8d

rpc: adjust helper contract code

73ecbb2

Compiler forces notification parameters to match the one specified in manifest, and manifest of `test_contract.go` tells that `to` parameter type is hash160: ``` event 'Transfer' should have 'Hash160' as type of 1 parameter, got: ByteArray ```

nativetest: migrate Management contract tests to neotest

e0ca05f

nativetest: migrate NEO contract tests to neotest

2f18b11

vm: optimise emit.Array() to use NEWARRAY0 for zero-lenght arrays

0e1f85b

nativetest: migrate Oracle contract tests to neotest

df6a7d4

nativetest: migrate Notary contract tests to neotest

ce5dfdb

AnnaShaleva force-pushed the tests-migration branch from 4982a9f to ce5dfdb Compare January 14, 2022 14:57

AnnaShaleva requested review from fyrchik and roman-khimov January 14, 2022 15:02

roman-khimov approved these changes Jan 14, 2022

View reviewed changes

roman-khimov merged commit 0b0531d into master Jan 14, 2022

roman-khimov deleted the tests-migration branch January 14, 2022 17:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

core: migrate native contract API tests to neotest framework #2299

core: migrate native contract API tests to neotest framework #2299

AnnaShaleva commented Dec 3, 2021 •

edited

Loading

roman-khimov Dec 6, 2021

AnnaShaleva Dec 15, 2021

AnnaShaleva commented Dec 9, 2021 •

edited

Loading

codecov bot commented Dec 15, 2021 •

edited

Loading

AnnaShaleva Dec 15, 2021

roman-khimov Jan 11, 2022

AnnaShaleva Jan 13, 2022

roman-khimov Jan 13, 2022

AnnaShaleva Dec 15, 2021

roman-khimov Dec 15, 2021

AnnaShaleva Dec 15, 2021

fyrchik Dec 20, 2021

fyrchik Dec 20, 2021

fyrchik Dec 20, 2021

roman-khimov Jan 11, 2022

AnnaShaleva commented Jan 13, 2022

roman-khimov left a comment

		h, err := e.Chain.GetNativeContractScriptHash(name)
		require.NoError(t, err)

core: migrate native contract API tests to neotest framework #2299

core: migrate native contract API tests to neotest framework #2299

Conversation

AnnaShaleva commented Dec 3, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AnnaShaleva commented Dec 9, 2021 • edited Loading

codecov bot commented Dec 15, 2021 • edited Loading

Codecov Report

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

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

AnnaShaleva commented Jan 13, 2022

roman-khimov left a comment

Choose a reason for hiding this comment

AnnaShaleva commented Dec 3, 2021 •

edited

Loading

AnnaShaleva commented Dec 9, 2021 •

edited

Loading

codecov bot commented Dec 15, 2021 •

edited

Loading