-
Notifications
You must be signed in to change notification settings - Fork 204
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Deep queries on metachain #5958
Conversation
factory/api/apiResolverFactory.go
Outdated
return nil, err | ||
var apiBlockchain data.ChainHandler | ||
|
||
if isMetachain { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would extract this in a small function so we can drop the else branch :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, fixed.
@@ -34,7 +34,7 @@ type GasScheduleMap = map[string]map[string]uint64 | |||
// TestNetwork wraps a set of TestProcessorNodes along with a set of test | |||
// Wallets, instantiates them, controls them and provides operations with them; | |||
// designed to be used in integration tests. | |||
// TODO combine TestNetwork with the preexisting TestContext and OneNodeNetwork | |||
// TODO combine TestNetwork with the preexisting TestContext and MiniNetwork |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only in the end I've seen that we have a TestNetwork
struct. Can we postpone the migration to TestNetwork
for another PR, or should we do it now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can postpone
|
||
require.Equal(t, snapshotsOfGetState[1], historyOfGetState[1]) | ||
require.Equal(t, snapshotsOfGetNow[1].blockNonce, historyOfGetNow[1].blockNonce) | ||
// This does not seem right! | ||
require.Equal(t, block4.GetRootHash(), historyOfGetNow[1].stateRootHash) | ||
|
||
require.Equal(t, snapshotsOfGetState[2], historyOfGetState[2]) | ||
require.Equal(t, snapshotsOfGetNow[2].blockNonce, historyOfGetNow[2].blockNonce) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed some assertions (see delta).
@@ -448,5 +449,24 @@ func TestCreateApiResolver_createScQueryElement(t *testing.T) { | |||
require.True(t, strings.Contains(strings.ToLower(err.Error()), "hasher")) | |||
require.Nil(t, scQueryService) | |||
}) | |||
} | |||
|
|||
func TestCreateApiResolver_createBlockchainForScQuery(t *testing.T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not very proud of this unit test.
Question for review: keep it or remove it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is ok. I would keep the tests :D
|
||
apiBlockchain, err := api.CreateBlockchainForScQuery(core.MetachainShardId) | ||
require.NoError(t, err) | ||
require.Equal(t, "*blockchain.metaChain", fmt.Sprintf("%T", apiBlockchain)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
This actually tests the factory characteristic of the function. It serves the purpose well.
@@ -448,5 +449,24 @@ func TestCreateApiResolver_createScQueryElement(t *testing.T) { | |||
require.True(t, strings.Contains(strings.ToLower(err.Error()), "hasher")) | |||
require.Nil(t, scQueryService) | |||
}) | |||
} | |||
|
|||
func TestCreateApiResolver_createBlockchainForScQuery(t *testing.T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is ok. I would keep the tests :D
chain handler
for SQ Query Service (metachain vs. shard).OneTestNetwork
- replace it withMiniNetwork
.