testing (rpc/modules): migrate current tests to integration tests, write new unit tests #1957
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov Report
@@ Coverage Diff @@
## development #1957 +/- ##
===============================================
+ Coverage 60.62% 61.23% +0.61%
===============================================
Files 202 203 +1
Lines 27286 27301 +15
===============================================
+ Hits 16541 16719 +178
+ Misses 8845 8706 -139
+ Partials 1900 1876 -24
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
jimjbrettj
force-pushed
the
jimmy/rpcModulesIntegrationTests
branch
from
535485d to
4fa800d
Compare
jimjbrettj
requested review from
arijitAD,
EclesioMeloJunior,
edwardmack,
kishansagathiya,
noot,
omar391 and
timwu20
as code owners
jimjbrettj
force-pushed
the
jimmy/rpcModulesIntegrationTests
branch
from
29aa003 to
653753b
Compare
jimjbrettj
force-pushed
the
jimmy/rpcModulesIntegrationTests
branch
from
653753b to
16ab592
Compare
noot
approved these changes
Nov 16, 2021
jimjbrettj
force-pushed
the
jimmy/rpcModulesIntegrationTests
branch
from
16ab592 to
95add11
Compare
yup yup! |
jimjbrettj
force-pushed
the
jimmy/rpcModulesIntegrationTests
branch
from
95add11 to
ebdc92e
Compare
qdm12
reviewed
Nov 30, 2021
| @@ -27,7 +27,6 @@ run: | |||
| skip-files: | |||
| - .*mock_.*\.go | |||
| - .*mocks\/.*\.go | |||
| - author_integration_test\.go # TODO remove once fixed | |||
| //go:build integration | ||
| // +build integration | ||
|
|
||
| // Copyright 2019 ChainSafe Systems (ON) Corp. |
| Hash: nil, | ||
| } | ||
|
|
||
| res := make([]string, 0) |
There was a problem hiding this comment.
nit
Suggested change
| res := make([]string, 0) | |
| var res []string |
Comment on lines
+51
to
+53
| require.Contains(t, []string{ | ||
| ":child_first", ":child_second", ":another_child", | ||
| }, string(b)) |
There was a problem hiding this comment.
That's risky since if b is always one of them, that will pass.
Instead change that to a set map[string]struct{}{":child_first": {}, ...} which you check with
key := string(b)
_, ok := set[key]
assert.True(t, ok)
delete(set, key)
Comment on lines
+120
to
+125
| var req GetChildStorageRequest | ||
| var res uint64 | ||
|
|
||
| req.Hash = test.hash | ||
| req.EntryKey = test.entry | ||
| req.KeyChild = test.keyChild |
There was a problem hiding this comment.
Suggested change
| var req GetChildStorageRequest | |
| var res uint64 | |
| req.Hash = test.hash | |
| req.EntryKey = test.entry | |
| req.KeyChild = test.keyChild | |
| req := GetChildStorageRequest{ | |
| Hash: test.hash, | |
| EntryKey: test.entry, | |
| KeyChild: test.keyChild, | |
| } | |
| var res uint64 | |
also why not have var req GetChildStorageRequest directly as a field in your test cases?
Comment on lines
+178
to
+184
| var req GetStorageHash | ||
| var res string | ||
|
|
||
| req.Hash = test.hash | ||
| req.EntryKey = test.entry | ||
| req.KeyChild = test.keyChild | ||
|
|
There was a problem hiding this comment.
same thing here, you could have req as a field in your test cases
|
|
||
| func TestGetChildStorage(t *testing.T) { | ||
| mod, blockHash := setupChildStateStorage(t) | ||
| randomHash, err := common.HexToHash(RandomHash) |
There was a problem hiding this comment.
What's RandomHash? Maybe have it inlined here as a constant? I don't think it's really random either, since it's a constant 🤔
Comment on lines
+238
to
+239
| require.Error(t, err) | ||
| require.Equal(t, err.Error(), test.errMsg) |
There was a problem hiding this comment.
Suggested change
| require.Error(t, err) | |
| require.Equal(t, err.Error(), test.errMsg) | |
| require.EqualError(t, err, test.errMsg) |
Comment on lines
+245
to
+249
| if test.expected != nil { | ||
| // Convert human-readable result value to hex. | ||
| expectedVal := "0x" + hex.EncodeToString(test.expected) | ||
| require.Equal(t, StateStorageResponse(expectedVal), res) | ||
| } |
There was a problem hiding this comment.
Should we check StateStorageResponse(expectedVal) is also as expected when test.expected is nil?
|
@EclesioMeloJunior I'm not sure where you are in refactoring these tests, but maybe this feedback could be helpful in your refactor |
|
@jimjbrettj thanks! @qdm12 the RPC integration tests are being touched in another PR (I will rise a PR for each RPC module). |
timwu20
force-pushed
the
jimmy/rpcModulesIntegrationTests
branch
from
025976c to
14f996f
Compare
…#1995) * migrate rpc/module tests to integration tests TODO:write unit tests * fix TestAuthorModule_InsertKey_Valid * fix TestAuthorModule_InsertKey_Valid_Gran_Keytype * migrate rpc/module tests to integration tests TODO:write unit tests * use mockery to generate BlockProducerAPI * WIP/test for dev control function * 100% test coverage for rpc/module/dev unit tests * test cov for getKeys in childState * 100% cover for getStorage size * 100% cover for getStorage hash * 100% coverage for childstate unit tests * format childState * lint * tests for offchain get * full coverage for offchain unit tests * unit tests for payment * 100% for grandpa proveFinality * grandpa unit tests * GenSyncSpec coverage * sync state unit tests * GetBlock * getBlockHash tests * chain tests * Wip/system tests * AccountNextIndex test * syncState test * tes localListenAndAddresses * test localPeerId * system tests * state getPairs test * getKeysPages test * test GetMetadata * test GetReadProof * test GetRuntime Version * test GetStorage * test GetStorageHash * test GetStorageSize * test queryStorage * state tests * remove subscribe func tests * format * rebase * fix naming and remove unnecessary variable inits * remove log * push test data * remove AnythingOfType from chain test * remove AnythingOfType from childState test * remove AnythingOfType from Grandpa test * remove AnythingOfType from offchain test * remove AnythingOfType from payment test * remove mocking anything in state test * remove mocking anything in sync state test * WIP/Finish removing mock.AnythingOfType * WIP/author test * author test * add exp cases to chain test * WIP/figure out error assertion issue * WIP/fix errors still * fix chain test error handling * author module tests fix * childstate test CR feedback * dev tests * wip/grandpa tests * grandpa tests * offchain tests * payment tests * syc state test updates * state test CR feedback * system tests * fix inconsistent test to use reflect on maps * use zip file for test data * delete test data hex file * fix up tests * close to final changes * finish CR feedback * clean up error checking * CR feedback TODO/Fix integration test error * move response init into subtests * remove panic from code * WIP/clean up tests * change res variable to be non pointer * WIP/add expected response check for error case * add response check for error case * finish first round of CR feedback * hasSessionKey tests * remove old author module tests * rename err to expErr * use system module constructor instead of fields * rename getter for test metadata * wip/CR feedback * separate out system getter tests * fix naming * clean up imports * wip/cr feedback * remove wantErr from rpc/module tests * finalize feedback * rebase * CR feedback * remove apimocks alias * final CR feedback * finalize CR changes * fix failing http test * fix LocalPeerId naming
timwu20
changed the title
timwu20
approved these changes
Dec 2, 2021
|
🎉 This PR is included in version 0.6.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
timwu20
pushed a commit
to timwu20/gossamer
that referenced
this pull request
Dec 6, 2021
…ite new unit tests (ChainSafe#1957) * fix TestAuthorModule_InsertKey_Valid * fix TestAuthorModule_InsertKey_Valid_Gran_Keytype * fix rpc/module/author_integration_test * migrate rpc/module tests to integration tests TODO:write unit tests * remove skip author integration tests from golangci.yml * lint * rebase and fix errors * license * testing (rpc/modules): write new unit tests to increase code coverage (ChainSafe#1995) * migrate rpc/module tests to integration tests TODO:write unit tests * fix TestAuthorModule_InsertKey_Valid * fix TestAuthorModule_InsertKey_Valid_Gran_Keytype * migrate rpc/module tests to integration tests TODO:write unit tests * use mockery to generate BlockProducerAPI * WIP/test for dev control function * 100% test coverage for rpc/module/dev unit tests * test cov for getKeys in childState * 100% cover for getStorage size * 100% cover for getStorage hash * 100% coverage for childstate unit tests * format childState * lint * tests for offchain get * full coverage for offchain unit tests * unit tests for payment * 100% for grandpa proveFinality * grandpa unit tests * GenSyncSpec coverage * sync state unit tests * GetBlock * getBlockHash tests * chain tests * Wip/system tests * AccountNextIndex test * syncState test * tes localListenAndAddresses * test localPeerId * system tests * state getPairs test * getKeysPages test * test GetMetadata * test GetReadProof * test GetRuntime Version * test GetStorage * test GetStorageHash * test GetStorageSize * test queryStorage * state tests * remove subscribe func tests * format * rebase * fix naming and remove unnecessary variable inits * remove log * push test data * remove AnythingOfType from chain test * remove AnythingOfType from childState test * remove AnythingOfType from Grandpa test * remove AnythingOfType from offchain test * remove AnythingOfType from payment test * remove mocking anything in state test * remove mocking anything in sync state test * WIP/Finish removing mock.AnythingOfType * WIP/author test * author test * add exp cases to chain test * WIP/figure out error assertion issue * WIP/fix errors still * fix chain test error handling * author module tests fix * childstate test CR feedback * dev tests * wip/grandpa tests * grandpa tests * offchain tests * payment tests * syc state test updates * state test CR feedback * system tests * fix inconsistent test to use reflect on maps * use zip file for test data * delete test data hex file * fix up tests * close to final changes * finish CR feedback * clean up error checking * CR feedback TODO/Fix integration test error * move response init into subtests * remove panic from code * WIP/clean up tests * change res variable to be non pointer * WIP/add expected response check for error case * add response check for error case * finish first round of CR feedback * hasSessionKey tests * remove old author module tests * rename err to expErr * use system module constructor instead of fields * rename getter for test metadata * wip/CR feedback * separate out system getter tests * fix naming * clean up imports * wip/cr feedback * remove wantErr from rpc/module tests * finalize feedback * rebase * CR feedback * remove apimocks alias * final CR feedback * finalize CR changes * fix failing http test * fix LocalPeerId naming
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Changes
Tests
Issues
Primary Reviewer