Skip to content

Remove getService() from TestKit #1184

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.

Sign up for GitHub

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

Merged
merged 8 commits into from
Oct 29, 2019
Merged

Remove getService() from TestKit #1184

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
082bae9
Remove getService() from TestKit
MakarovS Oct 25, 2019
1676a14
Minor fix
MakarovS Oct 25, 2019
3d84e00
Remove getNode() methods
MakarovS Oct 25, 2019
cab6698
Review fixes
MakarovS Oct 25, 2019
859f5fe
Add validator public key test
MakarovS Oct 26, 2019
4f8dee6
Minor fix
MakarovS Oct 26, 2019
2b1a67b
Add a comment
MakarovS Oct 28, 2019
0963579
Merge remote-tracking branch 'remotes/origin/ECR-3571' into remove_ge…
MakarovS Oct 28, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 0 additions & 17 deletions exonum-java-binding/testkit/src/main/java/com/exonum/binding/testkit/TestKit.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -169,23 +169,6 @@ public static TestKit forService(ServiceArtifactId artifactId, String artifactFi
.build();
}

/**
* Returns an instance of a service with the given service name and service class. Only
* user-defined services can be requested, i.e., it is not possible to get an instance of a
* built-in service such as the time oracle.
*
* @return the service instance or null if there is no service with such id
* @throws IllegalArgumentException if the service with given id was not found or could not be
* cast to given class
*/
public <T extends Service> T getService(String serviceName, Class<T> serviceClass) {
Service service = serviceRuntime.getServiceInstanceByName(serviceName);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

getServiceInstanceByName in the Runtime and ServiceWrapper are probably no longer needed.

checkArgument(service.getClass().equals(serviceClass),
"Service (name=%s, class=%s) cannot be cast to %s",
serviceName, service.getClass().getCanonicalName(), serviceClass.getCanonicalName());
return serviceClass.cast(service);
}

/**
* Creates a block with the given transaction(s). Transactions are applied in the lexicographical
* order of their hashes. In-pool transactions will be ignored.
Expand Down
2 changes: 1 addition & 1 deletion ...m-java-binding/testkit/src/test/java/com/exonum/binding/testkit/TestKitExtensionTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,7 @@ void beforeEachTestKitInject(TestKit testKit) {
instantiatedTestKit = testKit;

// Check that TestKit was instantiated with a correct service
TestService service = testKit.getService(SERVICE_NAME, TestService.class);
checkIfServiceEnabled(testKit, SERVICE_NAME, SERVICE_ID);

// Check that main TestKit node is a validator
assertThat(testKit.getEmulatedNode().getNodeType()).isEqualTo(EmulatedNodeType.VALIDATOR);
Expand Down
112 changes: 21 additions & 91 deletions exonum-java-binding/testkit/src/test/java/com/exonum/binding/testkit/TestKitTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
import static com.exonum.binding.testkit.TestService.THROWING_VALUE;
import static com.exonum.binding.testkit.TestService.constructAfterCommitTransaction;
import static com.exonum.binding.testkit.TestTransaction.BODY_CHARSET;
import static java.util.stream.Collectors.toList;
import static org.assertj.core.api.Assertions.assertThat;
import static org.junit.jupiter.api.Assertions.assertThrows;

Expand All @@ -33,14 +34,11 @@
import com.exonum.binding.core.blockchain.Block;
import com.exonum.binding.core.blockchain.Blockchain;
import com.exonum.binding.core.proxy.Cleaner;
import com.exonum.binding.core.runtime.DispatcherSchema;
import com.exonum.binding.core.service.Node;
import com.exonum.binding.core.storage.database.Snapshot;
import com.exonum.binding.core.storage.database.View;
import com.exonum.binding.core.storage.indices.MapIndex;
import com.exonum.binding.core.storage.indices.ProofMapIndexProxy;
import com.exonum.binding.core.transaction.RawTransaction;
import com.exonum.binding.messages.Runtime.InstanceSpec;
import com.exonum.binding.testkit.TestProtoMessages.TestConfiguration;
import com.exonum.binding.time.TimeSchema;
import com.google.common.collect.ImmutableList;
Expand Down Expand Up @@ -195,10 +193,9 @@ void createTestKitWithCustomConfiguration() {
.withService(ARTIFACT_ID, SERVICE_NAME, SERVICE_ID, testConfiguration)
.withArtifactsDirectory(artifactsDirectory)
.build()) {
TestService service = testKit.getService(SERVICE_NAME, TestService.class);
// Check that configuration value is used in initialization
Snapshot view = testKit.getSnapshot();
TestSchema testSchema = service.createDataSchema(view);
TestSchema testSchema = new TestSchema(view, SERVICE_ID);
ProofMapIndexProxy<HashCode, String> testProofMap = testSchema.testMap();
Map<HashCode, String> testMap = toMap(testProofMap);
Map<HashCode, String> expected = ImmutableMap.of(
Expand Down Expand Up @@ -262,16 +259,9 @@ void createTestKitWithTimeService() {
private void checkTestServiceInitialization(TestKit testKit, String serviceName, int serviceId) {
// Check that service appears in dispatcher schema
checkIfServiceEnabled(testKit, serviceName, serviceId);
TestService service = testKit.getService(serviceName, TestService.class);
// Check that TestService API is mounted
Node serviceNode = service.getNode();
EmulatedNode emulatedTestKitNode = testKit.getEmulatedNode();
assertThat(serviceNode.getPublicKey())
.isEqualTo(emulatedTestKitNode.getServiceKeyPair().getPublicKey());

// Check that initialization changed database state
Snapshot view = testKit.getSnapshot();
TestSchema testSchema = service.createDataSchema(view);
TestSchema testSchema = new TestSchema(view, serviceId);
ProofMapIndexProxy<HashCode, String> testProofMap = testSchema.testMap();
Map<HashCode, String> testMap = toMap(testProofMap);
Map<HashCode, String> expected = ImmutableMap.of(
Expand All @@ -286,30 +276,12 @@ private void checkTestServiceInitialization(TestKit testKit, String serviceName,
private void checkTestService2Initialization(TestKit testKit, String serviceName, int serviceId) {
// Check that service appears in dispatcher schema
checkIfServiceEnabled(testKit, serviceName, serviceId);
TestService2 service = testKit.getService(serviceName, TestService2.class);
// Check that TestService2 API is mounted
Node serviceNode = service.getNode();
EmulatedNode emulatedTestKitNode = testKit.getEmulatedNode();
assertThat(serviceNode.getPublicKey())
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we retrieve public key from core schema instead and validate them against emulatedTestKitNode public key or is that unnecessary?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have a separate test for getEmulatedNode? I think it makes sense to test it there using the core schema, but not here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Created a test for that, need a merged #1185 though

.isEqualTo(emulatedTestKitNode.getServiceKeyPair().getPublicKey());

// Check that genesis block was committed
Snapshot view = testKit.getSnapshot();
Blockchain blockchain = Blockchain.newInstance(view);
assertThat(blockchain.getBlockHashes().size()).isEqualTo(1L);
}

private void checkIfServiceEnabled(TestKit testKit, String serviceName, int serviceId) {
View view = testKit.getSnapshot();
MapIndex<String, InstanceSpec> serviceInstances =
new DispatcherSchema(view).serviceInstances();
assertThat(serviceInstances.containsKey(serviceName)).isTrue();

InstanceSpec serviceSpec = serviceInstances.get(serviceName);
int actualServiceId = serviceSpec.getId();
assertThat(actualServiceId).isEqualTo(serviceId);
}

@Test
void createTestKitWithSeveralValidators() {
short validatorCount = 2;
Expand Down Expand Up @@ -354,20 +326,6 @@ void setInvalidValidatorCount() {
assertThrows(exceptionType, () -> testKitBuilder.withValidators(invalidValidatorCount));
}

@Test
void getServiceWithWrongClassThrows(TestKit testKit) {
Class<IllegalArgumentException> exceptionType = IllegalArgumentException.class;
assertThrows(exceptionType,
() -> testKit.getService(SERVICE_NAME, TestService2.class));
}

@Test
void getServiceWithWrongNameThrows(TestKit testKit) {
Class<IllegalArgumentException> exceptionType = IllegalArgumentException.class;
assertThrows(exceptionType, () -> testKit.getService("Invalid service name",
TestService.class));
}

@Test
void createTestKitMoreThanMaxServiceNumber() {
Class<IllegalArgumentException> exceptionType = IllegalArgumentException.class;
Expand Down Expand Up @@ -439,6 +397,24 @@ void createBlockWithTransactionIgnoresInPoolTransactions(TestKit testKit) {
assertThat(inPoolTransactions).hasSize(2);
}

@Test
void getTransactionPool(TestKit testKit) {
// Create two blocks with no transactions
Block block1 = testKit.createBlock();
Block block2 = testKit.createBlockWithTransactions();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why the difference (createBlock vs createBlockWithTransactions), the comment above says no transactions?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's an unclear (from the source) distinction that warrants an explanation in the source, because it is relied upon.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Javadoc of this method contains this distinction - In-pool transactions will be ignored.

Added an additional comment for this line.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Javadoc does not show the intent of the writer of this method.

// Two blocks were created, so two afterCommit transactions should be submitted into pool
List<TransactionMessage> transactionsInPool = testKit.getTransactionPool();
RawTransaction afterCommitTransaction1 =
constructAfterCommitTransaction(SERVICE_ID, block1.getHeight());
RawTransaction afterCommitTransaction2 =
constructAfterCommitTransaction(SERVICE_ID, block2.getHeight());
List<RawTransaction> rawTransactionsInPool = transactionsInPool.stream()
.map(RawTransaction::fromMessage)
.collect(toList());
assertThat(rawTransactionsInPool)
.containsExactlyInAnyOrder(afterCommitTransaction1, afterCommitTransaction2);
}

@Test
void createBlockWithSingleTransaction(TestKit testKit) {
TransactionMessage message = constructTestTransactionMessage("Test message");
Expand Down Expand Up @@ -467,52 +443,6 @@ void createBlockWithTransactions(TestKit testKit) {
view, block, message, message2));
}

@Test
void nodeSubmittedTransactionsArePlacedInPool(TestKit testKit) {
TestService service = testKit.getService(SERVICE_NAME, TestService.class);

TransactionMessage message = constructTestTransactionMessage("Test message", testKit);
RawTransaction rawTransaction = RawTransaction.fromMessage(message);
service.getNode().submitTransaction(rawTransaction);

List<TransactionMessage> transactionsInPool =
testKit.findTransactionsInPool(tx -> tx.getServiceId() == SERVICE_ID);
assertThat(transactionsInPool).isEqualTo(ImmutableList.of(message));
}

@Test
void getTransactionPool(TestKit testKit) {
TestService service = testKit.getService(SERVICE_NAME, TestService.class);

TransactionMessage message = constructTestTransactionMessage("Test message", testKit);
RawTransaction rawTransaction = RawTransaction.fromMessage(message);
TransactionMessage message2 = constructTestTransactionMessage("Test message 2", testKit);
RawTransaction rawTransaction2 = RawTransaction.fromMessage(message2);

service.getNode().submitTransaction(rawTransaction);
service.getNode().submitTransaction(rawTransaction2);

List<TransactionMessage> transactionsInPool = testKit.getTransactionPool();
assertThat(transactionsInPool).containsExactlyInAnyOrder(message, message2);
}

@Test
void findTransactionsInPool(TestKit testKit) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TestKit.findTransactionsInPool() is tested in afterCommitSubmitsTransaction test.

TestService service = testKit.getService(SERVICE_NAME, TestService.class);

TransactionMessage message = constructTestTransactionMessage("Test message", testKit);
RawTransaction rawTransaction = RawTransaction.fromMessage(message);
TransactionMessage message2 = constructTestTransactionMessage("Test message 2", testKit);
RawTransaction rawTransaction2 = RawTransaction.fromMessage(message2);
service.getNode().submitTransaction(rawTransaction);
service.getNode().submitTransaction(rawTransaction2);

List<TransactionMessage> transactionsInPool =
testKit.findTransactionsInPool(
tx -> tx.getPayload().equals(message.getPayload()));
assertThat(transactionsInPool).containsExactly(message);
}

@Test
void createBlockWithTransactionsVarargs(TestKit testKit) {
TransactionMessage message = constructTestTransactionMessage("Test message");
Expand Down
17 changes: 17 additions & 0 deletions ...ing/testkit/src/test/java/com/exonum/binding/testkit/TestKitTestWithArtifactsCreated.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,13 @@

package com.exonum.binding.testkit;

import static org.assertj.core.api.Assertions.assertThat;

import com.exonum.binding.core.runtime.DispatcherSchema;
import com.exonum.binding.core.runtime.ServiceArtifactId;
import com.exonum.binding.core.storage.database.View;
import com.exonum.binding.core.storage.indices.MapIndex;
import com.exonum.binding.messages.Runtime.InstanceSpec;
import com.exonum.binding.test.runtime.ServiceArtifactBuilder;
import com.exonum.binding.testkit.TestProtoMessages.TestConfiguration;
import java.io.IOException;
Expand Down Expand Up @@ -72,6 +78,17 @@ static void createInvalidArtifact(Path directory, String filename) throws IOExce
TestSchema.class);
}

static void checkIfServiceEnabled(TestKit testKit, String serviceName, int serviceId) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we create separate TestKitTestUtils and place it there or is this class good enough? If so, rename this class?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It could live here, I think.

View view = testKit.getSnapshot();
MapIndex<String, InstanceSpec> serviceInstances =
new DispatcherSchema(view).serviceInstances();
assertThat(serviceInstances.containsKey(serviceName)).isTrue();

InstanceSpec serviceSpec = serviceInstances.get(serviceName);
int actualServiceId = serviceSpec.getId();
assertThat(actualServiceId).isEqualTo(serviceId);
}

private static void createArtifact(Path artifactLocation, ServiceArtifactId serviceArtifactId,
Class serviceModule,
Class<?>... artifactClasses) throws IOException {
Expand Down