Add TestKit [ECR-3049]

[MakarovS]

Overview

Add TestKit P1 functionality.

---

See: https://jira.bf.local/browse/ECR-3049

Definition of Done

• There are no TODOs left in the code
• Change is covered by automated tests
• The coding guidelines are followed
• Public API has Javadoc
• Method preconditions are checked and documented in the Javadoc of the method
• Changelog is updated if needed (in case of notable or breaking changes) - will be updated in a later PR
• The continuous integration build passes

[bullet-tooth]

/** 
* Type of the Exonum node emulated by TestKit.
*
* @see <a href="https://exonum.com/doc/version/0.10/glossary/#auditor">Auditor Node</a>
*      <a href="https://exonum.com/doc/version/0.10/glossary/#validator">Validator Node</a>
**/

[bullet-tooth]

javadoc is missing. Please describe the purpose of this class.

[dmitry-timofeev]

Or is it decided that it is better to add this just as a placeholder and implement in a separate PR, as originally planned?

[MakarovS]

Yes, sorry for half-introducing classes from other tasks - I wanted native methods signatures to be ready for native tasks, so created these classes as placeholders

[dmitry-timofeev]

Let's keep them as placeholders to avoid redundant work then.

[bullet-tooth]

Suggested change

	private final static Injector frameworkInjector = Guice.createInjector(new TestKitFrameworkModule());
	private static final Injector frameworkInjector = Guice.createInjector(new TestKitFrameworkModule());

[bullet-tooth]

Suggested change

	final static short MAX_SERVICE_NUMBER = 256;
	static final short MAX_SERVICE_NUMBER = 256;

[bullet-tooth]

Suggested change

	List<UserServiceAdapter> serviceAdapters = new ArrayList<>();
	List<UserServiceAdapter> serviceAdapters = new ArrayList<>(serviceModules.size());

[bullet-tooth]

Suggested change

	checkArgument(service.getClass().equals(serviceClass), "Service with id %s is not of expected class %s",
	checkArgument(service.getClass().equals(serviceClass), "Service with id %s is not an instance of expected class %s",

[bullet-tooth]

Suggested change

	checkArgument(moduleObject instanceof Module, "%s is not a sub-class of %s",
	checkArgument(moduleObject instanceof Module, "%s is not a sub-class of the %s",

Btw, looks like this check is always true because ServiceModule extends Module. No? I believe we can cast to ServiceModule at initialization and remove this check.

[dmitry-timofeev]

This code seems to be copied from the place where we instantiate a class and subsequently a module by name, therefore, some checks can be redundant.

I'd also look into multiple-services branch. There might be something reusable here.

[dmitry-timofeev]

com.exonum.binding.runtime.ReflectiveModuleSupplier

[bullet-tooth]

Is it allowed to have 255 services, not 256?

[dmitry-timofeev]

if the constant is named max, it must be inclusive

[bullet-tooth]

javadoc

[dmitry-timofeev]

Or in time-PR?

[bullet-tooth]

List.of(...) or ImmutableList.of()

[dmitry-timofeev]

Generally good, thanks! There are some comments re docs and tests

[dmitry-timofeev]

if an auditor node.

[dmitry-timofeev]

I wonder if there is a good place to link in this case (Node#getPublicKey — but it only returns the public key;
Node#submitTransaction — it signs a transaction with the service public key; or ValidatorKey#consensusKey — but its
documentation is not particularly detailed).

At least, I'd add here in which operations this key pair is used, e.g., "This key pair is used to sign transactions {@linkplain Node#submitTransaction produced} by the service itself".

[dmitry-timofeev]

I'd add that this key pair is used to sign consensus message of this node.

[dmitry-timofeev]

Or is it decided that it is better to add this just as a placeholder and implement in a separate PR, as originally planned?

[dmitry-timofeev]

maybe specify that a single instance per service is created, e.g.:

"... only one node will create the service instances and will execute their operations (e.g., ...)"

[dmitry-timofeev]

Is there anything else that must be validated? E.g., non-empty list of services?

[MakarovS]

I added such a check, even though it's possible to create a blockchain without any user services and if users want to run such a network (only with time service, for example), maybe we should let them, even if it's useless? But probably it's better to fail in such case to indicate that a user forgot to add a service

[dmitry-timofeev]

Or in time-PR?

[dmitry-timofeev]

Shan't we test the other side-effects of creating a testkit (= service initialization, mounting of web handlers)?
I think we can do that with a service implementation that produces some effects in its #initialize and returns
some value.

[MakarovS]

I was planning to add that later, with the native part, rather than writing and ignoring them now. Should I do the latter?

[dmitry-timofeev]

I think either is OK, if we don't forget to add the proper tests/update the already added to test all aspects.

[MakarovS]

Ok, I'll create a separate task for that then.

[dmitry-timofeev]

Shall we allow that? With the present framework version it will not work at all.

[MakarovS]

IIRC there was a discussion on wiki some time ago stating that we should, but that could change. What's the current outlook on having several instances of the same service?

[dmitry-timofeev]

Even with multiple services, it is not possible to create two instances of the same service because their database schemas will be the same; their ids will be the same (so the testkit will likely fail somewhere in native). Dynamic services will require providing instance parameters — i.e., something on top of a ServiceModule (e.g., a service instance name, or other initialization parameters), therefore the present interface won't work with them.

[dmitry-timofeev]

I think since #818 it must be slightly different, @vitvakatu can advise

[vitvakatu]

It's okay, as we don't have tests with release mode at the moment

[skletsun]

Nit: Plural for "transactions" in the comment for .getServiceKeyPair() and singular for "consensus message" here.

[vitvakatu]

We always call createPublicApiHandlers, even if no handlers are needed. I wonder if constructing this service will lead to panic

[vitvakatu]

Should we mention genesis block creation? That's all I believe

[coveralls]

Coverage decreased (-0.3%) to 85.742% when pulling 88cbab6 on ECR-3049 into 492d956 on master.

[dmitry-timofeev]

@vitvakatu, Would you clarify please what is the exact order? Do we initialize after
genesis block? When do we create public API handlers?

[vitvakatu]

The order is initialize -> genesis block commit -> create handlers. Service::initialize is called on genesis block creation, so actual it looks more like initialize and genesis block commit -> create handlers

[dmitry-timofeev]

Thanks, it seems this description needs to be updated to reflect the actual order of operations.

[dmitry-timofeev]

+ "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."

[dmitry-timofeev]

It is not appropriate to throw NPE when it is not the client passing nulls. IllegalArgumentException would be more appropriate (checkArgument(services.containsKey…)

[dmitry-timofeev]

Instantiates a service given its module and wraps in a UserServiceAdapter
for the native code.

(we don't create a USA from the module, but a service, and then wrap it).

[dmitry-timofeev]

These methods are used by the constructor only, therefore, I'd move them closer to it

• ctor
  • toUserServiceAdapters
    • createUserServiceAdapter
  • populateServiceMap

[dmitry-timofeev]

If they are private, have only a single usage and are adjacent to it, some documentation comments might no longer be needed (unless they clarify something possibly unclear).

[dmitry-timofeev]

… and checkForDuplicateService obviously

[dmitry-timofeev]

Suggested change

	return serviceAdapters;
	return serviceModules.stream()
	.map(this::createUserServiceAdapter)
	.collect(Collectors.toList());

[dmitry-timofeev]

with static imports, probably, as @bullet-tooth would recommend :-)

[dmitry-timofeev]

Suggested change

	private void checkForDuplicateService(short serviceId) {
	private void checkForDuplicateService(UserServiceAdapter newService) {
	short serviceId = newService.getId();
	checkArgument(!services.containsKey(serviceId),
	"Service with id %s was added to the TestKit twice: %s and %s",
	serviceId, services.get(serviceId), newService.getService());
	}

[dmitry-timofeev]

Otherwise it might not be clear which, ids are just numbers

[dmitry-timofeev]

Here and below: omit the type (below it is no longer correct as any iterable is allowed, not just list): "Adds services with which the TestKit would be instantiated."

[dmitry-timofeev]

It seems to qualify for @SafeVarargs, but double-check that

[dmitry-timofeev]

I find this one a little clearer (addAll instead of generic for each):

Suggested change

	serviceModules.forEach(services::add);
	Iterables.addAll(services, serviceModules);

[dmitry-timofeev]

Suggested change

	return this;
	return withServices(asList(serviceModule, serviceModules));

[bullet-tooth]

👍

[bullet-tooth]

final is redundant for methods such as the class is final.

[dmitry-timofeev]

Yep, I think it is only required (even in final classes) for @SafeVarargs methods.

[MakarovS]

Yes, it was needed for @SafeVarargs method, so I added others for consistency.
Edit: removed for all except for @SafeVarargs method

[bullet-tooth]

actually, serviceModule param can be removed and java.util.Arrays#asList is used as the varargs wrapper. Or did I misunderstand something?)

[dmitry-timofeev]

Yeah, it does not seem to be needed for resolution of methods with ambiguious signatures (the one above has different name). However, it does communicate a requirement of at least one service module.

[bullet-tooth]

a requirement of at least one service module
There is withService in this case.

[MakarovS]

Are there any disadvantages to the present signature? If not, I'd leave it as it is

[bullet-tooth]

Cryptocurrency service has id=42 as well. But I think it's not a problem.

[MakarovS]

Yes, I thought about that, but that shouldn't be a problem
Edit: changed anyway :)

[MakarovS]

It appears that in native we can't get a private consensus key, so after discussion with @vitvakatu we decided to remove consensus keys altogether (even though the public key is available), as it seems consensus keys would be not necessary for users. But we could give access to consensus public key or even change the core and make it possible to access both public and private consensus keys. Does anyone sees any usefulness in those? @dmitry-timofeev @bullet-tooth

[dmitry-timofeev]

I don't see immediate usefulness in the consensus public key access. You can validate precommits and other consensus messages with it — but why would you?