Remove getService() from TestKit #1184
Conversation
| } | ||
|
|
||
| @Test | ||
| void findTransactionsInPool(TestKit testKit) { |
There was a problem hiding this comment.
TestKit.findTransactionsInPool() is tested in afterCommitSubmitsTransaction test.
| TestSchema.class); | ||
| } | ||
|
|
||
| static void checkIfServiceEnabled(TestKit testKit, String serviceName, int serviceId) { |
There was a problem hiding this comment.
Should we create separate TestKitTestUtils and place it there or is this class good enough? If so, rename this class?
There was a problem hiding this comment.
It could live here, I think.
| // Check that TestService2 API is mounted | ||
| Node serviceNode = service.getNode(); | ||
| EmulatedNode emulatedTestKitNode = testKit.getEmulatedNode(); | ||
| assertThat(serviceNode.getPublicKey()) |
There was a problem hiding this comment.
Should we retrieve public key from core schema instead and validate them against emulatedTestKitNode public key or is that unnecessary?
There was a problem hiding this comment.
Do we have a separate test for getEmulatedNode? I think it makes sense to test it there using the core schema, but not here.
There was a problem hiding this comment.
We do, but not using core schema (we just check that the node type (validator or auditor) is correct and keys are not null). Should I put this PR on WIP until #1185 is merged and then add the full test?
There was a problem hiding this comment.
Created a test for that, need a merged #1185 though
| * cast to given class | ||
| */ | ||
| public <T extends Service> T getService(String serviceName, Class<T> serviceClass) { | ||
| Service service = serviceRuntime.getServiceInstanceByName(serviceName); |
There was a problem hiding this comment.
getServiceInstanceByName in the Runtime and ServiceWrapper are probably no longer needed.
| // Check that TestService2 API is mounted | ||
| Node serviceNode = service.getNode(); | ||
| EmulatedNode emulatedTestKitNode = testKit.getEmulatedNode(); | ||
| assertThat(serviceNode.getPublicKey()) |
There was a problem hiding this comment.
Do we have a separate test for getEmulatedNode? I think it makes sense to test it there using the core schema, but not here.
| // Create two blocks with no transactions, so two afterCommit transactions are stored in | ||
| // the transaction pool | ||
| Block block1 = testKit.createBlock(); | ||
| Block block2 = testKit.createBlockWithTransactions(); |
There was a problem hiding this comment.
Why the difference (createBlock vs createBlockWithTransactions), the comment above says no transactions?
There was a problem hiding this comment.
If we use createBlock for second time it would create a block with the first afterCommit transaction from pool, and we want to keep it there - that's why createBlockWithTransactions
There was a problem hiding this comment.
That's an unclear (from the source) distinction that warrants an explanation in the source, because it is relied upon.
There was a problem hiding this comment.
Javadoc of this method contains this distinction - In-pool transactions will be ignored.
Added an additional comment for this line.
There was a problem hiding this comment.
Javadoc does not show the intent of the writer of this method.
| TestSchema.class); | ||
| } | ||
|
|
||
| static void checkIfServiceEnabled(TestKit testKit, String serviceName, int serviceId) { |
There was a problem hiding this comment.
It could live here, I think.
3 participants
Overview
Remove
<T extends Service> T getService(String serviceName, Class<T> serviceClass)from TestKit in favour of testing read requests to service via service schema.See: https://jira.bf.local/browse/ECR-3723
Definition of Done