Add TestKit JUnit extension [ECR-3076] #913

MakarovS · 2019-05-27T04:48:45Z

Overview

Add TestKit JUnit extension.

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

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)
The continuous integration build passes

coveralls · 2019-05-27T05:12:40Z

Coverage increased (+0.1%) to 85.586% when pulling 9164ddd on ECR-3076 into d430654 on master.

dmitry-timofeev · 2019-05-27T15:00:53Z

exonum-java-binding/testkit/src/test/java/com/exonum/binding/testkit/TestKitExtension.java

+  }
+
+  @Override
+  public void beforeEach(ExtensionContext extensionContext) {


Why beforeEach is implemented, creating the Testkit even if it is not requested? I think it must be instantiated when requested (= resolved).

dmitry-timofeev · 2019-05-27T15:01:56Z

exonum-java-binding/testkit/src/test/java/com/exonum/binding/testkit/TestKitExtension.java

+  @Override
+  public void afterEach(ExtensionContext extensionContext) {
+    TestKit testKit = getStore(extensionContext).get(KEY, TestKit.class);
+    testKit.disposeInternal();


Why do we need this if the store will destroy the objects put there, if they implement https://junit.org/junit5/docs/current/api/org/junit/jupiter/api/extension/ExtensionContext.Store.CloseableResource.html?

dmitry-timofeev · 2019-05-27T15:15:40Z

exonum-java-binding/testkit/src/test/java/com/exonum/binding/testkit/TestKitExtension.java

+
+  private ExtensionContext.Store getStore(ExtensionContext context) {
+    return context.getStore(ExtensionContext.Namespace.create(getClass(),
+        context.getRequiredTestMethod()));


Is it really needed to include the test method as the namespace? Can't we just create a single namespace as a constant? I think stores of, for instance, test methods running in parallel isolated, and TempDirectory in JUnit 5 suggests they are.

dmitry-timofeev · 2019-05-27T15:34:22Z

exonum-java-binding/testkit/src/test/java/com/exonum/binding/testkit/TestKitTest.java

-      service = testKit.getService(TestService.SERVICE_ID, TestService.class);
-      checkTestServiceInitialization(testKit, service);
-    }
+  void createTestKitForSingleService(TestKit testKit) {


It is nice if we can inject Testkit directly, but the present implementation makes it impossible to configure the testkit differently for different tests (without @Nested or separating the tests into multiple files).

Yes, I thought about that, but is it a frequent case for users to test their service with different TestKit configuration? It seems that usually they'd use the same configuration in the same test suite or use @Nested. Even if they need that, they could just not inject it and create it manually. If not, what do you propose?

auditor/validator?

If not, what do you propose?

Please see in jira.

dmitry-timofeev · 2019-05-29T13:32:14Z

exonum-java-binding/testkit/src/main/java/com/exonum/binding/testkit/TestKit.java

+     * {@link Service#afterCommit(BlockCommittedEvent)} logic will only be called on the main
+     * TestKit node of this type
+     */
+    public Builder withNodeType(EmulatedNodeType nodeType) {


Shall we drop the node type argument from builder method, and just make 'validator' the default?

dmitry-timofeev · 2019-05-29T13:33:46Z

exonum-java-binding/testkit/src/test/java/com/exonum/binding/testkit/TestKitExtension.java

  private static final String KEY = "ResourceKey";
+  private static final Set<Class> testKitModificationAnnotations =
+      ImmutableSet.of(Auditor.class, Validator.class, ValidatorCount.class,
+          WithoutTimeService.class);


WithTimeService(true/false)?

dmitry-timofeev · 2019-05-29T13:35:46Z

exonum-java-binding/testkit/src/test/java/com/exonum/binding/testkit/TestKitExtension.java

+      // reconfigure it
+      if (annotationsUsed) {
+        throw new RuntimeException("TestKit was parameterized with annotations after being"
+            + " instantiated in " + extensionContext.getDisplayName());


I am not sure "after being instantiated in " is correct. It was instantiated sometime earlier (in @BeforeEach, for instance).

For better reporting/debuggability, I think it is possible to include info about the 'origin' injection (= testkit instantiation) point in the CloseableTestKit.

Sorry for the ambiguity - I intended this message to mean "parameterized in test", not "instantiated in test".
The thing is that we can inject either in @BeforeEach, @Test, or @AfterEach. All of them have the same ExtensionContext, so it's not clear what to put as an origin.
There are two possible options -

if a user reconfigures TestKit in @Test, then he will get a clear displayName

If he tries to reconfigure in @AfterEach then he'll get displayNames of all tests he has.

We could improve the exception message though and make clear what could be incorrect.

dmitry-timofeev · 2019-05-29T13:36:48Z

exonum-java-binding/testkit/src/test/java/com/exonum/binding/testkit/TestKitExtension.java

+      // Throw an exception if TestKit was already instantiated in this context, but user tries to
+      // reconfigure it
+      if (annotationsUsed) {
+        throw new RuntimeException("TestKit was parameterized with annotations after being"


A more specific exception is needed: will ParameterResolutionException work (please check its documentation)?

dmitry-timofeev · 2019-05-29T13:38:24Z

exonum-java-binding/testkit/src/test/java/com/exonum/binding/testkit/TestKitExtension.java

+   */
+  private void checkExtensionContext(ExtensionContext extensionContext) {
+    Optional<Method> testMethod = extensionContext.getTestMethod();
+    testMethod.orElseThrow(() ->


😕 I don't see that the actual condition corresponds to the intended one.

Also, unless the extension is in static field, it won't be able to inject in @BeforeAll — so we may consider dropping this limitation, just documenting the consequences in the extension documentation.

I don't see that the actual condition corresponds to the intended one.

What do you mean? For @BeforeAll and @AfterAll it will be empty, for @BeforeEach, @Test, @AfterEach it will contain a test method.

Also, unless the extension is in static field, it won't be able to inject in @BeforeAll — so we may consider dropping this limitation, just documenting the consequences in the extension documentation.

Unless @TestInstance(Lifecycle.PER_CLASS) is used, then user is able to declare @BeforeAll and @AfterAll as non-static.

I meant that the code said getTestMethod but the condition referred to BeforeAll/AfterAll, and this connection was not obvious from the code, as the fact that getTestMethod returned non-empty thing in case of BeforeEach/AfterEach.

I suggest clarifying that concisely.

dmitry-timofeev · 2019-05-29T13:45:30Z

exonum-java-binding/testkit/src/test/java/com/exonum/binding/testkit/TestKitExtension.java

+
+    Optional<WithoutTimeService> withTimeServiceAnnotation =
+        parameterContext.findAnnotation(WithoutTimeService.class);
+    withTimeServiceAnnotation.ifPresent(withoutTimeService ->


If it needs a time provider which cannot be provided via an annotation, then I think we simply shall not support such configuration.

dmitry-timofeev · 2019-05-29T13:57:23Z

exonum-java-binding/testkit/src/test/java/com/exonum/binding/testkit/TestKitExtension.java

+    CloseableTestKit closeableTestKit =
+        getStore(extensionContext).get(KEY, CloseableTestKit.class);
+    // Check if any TestKit configuration annotations are used
+    boolean annotationsUsed = annotationsUsed(parameterContext);


What if we hide this thing inside buildTestkit and the other branch:

if (closeableTestKit == null) { testkit = buildTestkit(parameterContext, extensionContext); getStore(extensionContext).put(KEY, new CloseableTestKit(testkit)); } else { if (annotationsUsed(parameterContext)) { fail } testkit = closeableTestkit.getTestkit(); } return testkit;

with buildTestkit

overrideDefaults()/updateTestkitBuilder() — the implementation of that may check if annotations are present itself return builder.build();

dmitry-timofeev · 2019-05-29T13:58:35Z

exonum-java-binding/testkit/src/test/java/com/exonum/binding/testkit/TestKitExtension.java

  private final TestKit.Builder testKitBuilder;

  public TestKitExtension(TestKit.Builder testKitBuilder) {
    this.testKitBuilder = testKitBuilder;


templateTestkitBuilder/templateBuilder — as it is used as a template to create new testkits; and add documentation explaining you can override defaults with the following annotations: @Foo, @Bar, @Baz

dmitry-timofeev · 2019-05-29T14:06:07Z

exonum-java-binding/testkit/src/test/java/com/exonum/binding/testkit/ValidatorCount.java

+public @interface ValidatorCount {
+
+  /**
+   * Validator count of TestKit network.


Number of validator nodes in the Testkit network. Must be positive. @see Builder#<apropriate method>

dmitry-timofeev · 2019-05-29T14:06:30Z

exonum-java-binding/testkit/src/test/java/com/exonum/binding/testkit/Validator.java

+import java.lang.annotation.Target;
+
+/**
+ * Changes main TestKit node type to validator for injected TestKit.


@see <appropriate enum/Builder method>?

dmitry-timofeev · 2019-05-30T15:07:50Z

exonum-java-binding/testkit/src/test/java/com/exonum/binding/testkit/TestKitExtension.java

+  private void checkExtensionContext(ExtensionContext extensionContext) {
+    Optional<Method> testMethod = extensionContext.getTestMethod();
+    testMethod.orElseThrow(() ->
+        new RuntimeException("TestKit can't be injected in @BeforeAll or @AfterAll"));


On the question of whether we shall or shall not forbid it:

"If you do need the same Testkit instance for all tests in the class, just use the builder directly:

private static final Testkit testkit = Testkit.newBuilder()
.withService(YourServiceModule.class)
.build();
"

… but as I wrote this instruction I remembered that the user will also need to write @AfterAll that destroys the testkit! So maybe we shall support injection in BeforeAll after all, shan't we?

Yes, but isn't the idea of @Test cases is that they are independent? Why would a user want to use the same TestKit? It's usually used to create some blocks and then review the state of the system, so using the same TestKit would lead to errors.

But we could document that and it would be easier for us - no need to throw an exception in @Before/AfterAll injections.

So the questions are:

Could anyone (@bullet-tooth) think of a realistic use-case when one needs a shared testkit for all methods?

Shall we restrict such usage (as error-prone) if we can't think of such use-cases?

I can think of a scenario when setup is performed in @BeforeAll and all tests perform only read-only operations on testkit — but how realistic that is, and how resistant to future additions of new tests performing write operations? Also, this scenario would also work with @BeforeEach, somewhat slower, but way more reliable.

Also I'd prefer to explicitly prohibit injecting in @AfterAll, as it doesn't really make sense (it'll just create one more TestKit if injected). Although I agree that users might want to perform only read-only operations in different tests, it seems like a very error-prone approach. I think @BeforeEach is more reliable indeed even if slow.

it'll just create one more TestKit if injected

It won't — contexts are nested, everything in the parent context (@BeforeAll) is accessible in children (@Beach, @Test).

Ok, let's forbid but let's put something along the lines of

Testkit cannot be injected in @BeforeAll/AfterAll because it is a stateful, mutable, object and sharing it between all tests is error-prone. Consider injecting it in @BeforeEach instead. If you *do* need the same instance for all tests — just use `Testkit#builder` directly. Don't forget to destroy it in @AfterEach.

It won't — contexts are nested, everything in the parent context (@BeforeAll) is accessible in children (@beach, @test).

I was talking about @AfterAll. If we inject in @BeforeAll, we'd have same TestKit in all tests - but if we inject in @AfterAll, we won't - every Test will have its own instance, including @AfterAll method.

I see, in this case — yes, but it makes no apparent sense to use that :-)

dmitry-timofeev · 2019-06-03T13:36:41Z

exonum-java-binding/testkit/src/main/java/com/exonum/binding/testkit/Auditor.java

 /**
 * Changes main TestKit node type to auditor for injected TestKit.
+ *
+ * @see TestKit.Builder#withNodeType(EmulatedNodeType)


Is second @see missing? Because the method won't show.

dmitry-timofeev · 2019-06-03T13:37:00Z

exonum-java-binding/testkit/pom.xml

    </dependency>

+    <dependency>
+      <groupId>junit</groupId>


It is JUnit 4.

dmitry-timofeev · 2019-06-03T13:37:24Z

exonum-java-binding/testkit/pom.xml

+    <dependency>
+      <groupId>org.junit.platform</groupId>
+      <artifactId>junit-platform-testkit</artifactId>
+      <version>${junit-platform-testkit.version}</version>


test-scoped?

dmitry-timofeev · 2019-06-03T13:39:47Z

exonum-java-binding/testkit/src/main/java/com/exonum/binding/testkit/TestKitExtension.java

+ *
+ * <pre><code>
+ * static {
+ *   LibraryLoader.load();


It is no longer needed (once the pr lands)

dmitry-timofeev · 2019-06-03T13:40:02Z

exonum-java-binding/testkit/src/main/java/com/exonum/binding/testkit/TestKitExtension.java

+ * Allows injecting TestKit into test context:
+ *
+ * <pre><code>
+ * @RegisterExtension


@ must be escaped

dmitry-timofeev · 2019-06-03T13:45:04Z

exonum-java-binding/testkit/src/test/java/com/exonum/binding/testkit/TestKitTest.java

+import org.junit.jupiter.api.TestInstance.Lifecycle;
 import org.junit.jupiter.api.extension.RegisterExtension;

+@TestInstance(Lifecycle.PER_CLASS)


Why? It does not seem to be needed.

dmitry-timofeev · 2019-06-03T13:45:23Z

exonum-java-binding/testkit/src/test/java/com/exonum/binding/testkit/TestKitTest.java

  @RegisterExtension
-  TestKitExtension testKitExtension = new TestKitExtension(
-      TestKit.builder(EmulatedNodeType.VALIDATOR)
+  static TestKitExtension testKitExtension = new TestKitExtension(


Shall we use the extension in the test of testkit + its builder?

I think it's okay to use extension for most of the TestKit tests (not builder tests though).

dmitry-timofeev · 2019-06-03T13:45:45Z

exonum-java-binding/testkit/src/test/java/com/exonum/binding/testkit/TestKitTest.java

  }

  @Test
  void createTestKitWithBuilderForSingleService() {


It is probably no longer needed, isn't it?

But why? I think it's still a viable test.

Sorry, wrong method — the question is for createTestKitForSingleService^.

dmitry-timofeev · 2019-06-03T13:47:56Z

exonum-java-binding/testkit/src/test/java/com/exonum/binding/testkit/TestKitTest.java

    short validatorCount = 2;
-    try (TestKit testKit = TestKit.builder(EmulatedNodeType.AUDITOR)
+    try (TestKit testKit = TestKit.builder()
+        .withNodeType(EmulatedNodeType.AUDITOR)


Shall we use with prefix, or maybe drop it (e.g., nodeType) or add usual set and add depending on the operation? On the other hand, if used consistently, it slightly helps with discoverability (if all use with, you can type wi and see what it could do).

I like having same prefix for the reason you stated - it's nice to have all methods starting with with

dmitry-timofeev · 2019-06-03T13:48:33Z

exonum-java-binding/testkit/src/main/java/com/exonum/binding/testkit/Validator.java

 /**
 * Changes main TestKit node type to validator for injected TestKit.
+ *
+ * @see TestKit.Builder#withNodeType(EmulatedNodeType)


Same as above (two @sees?)

...cy-demo/src/test/java/com/exonum/binding/cryptocurrency/transactions/CreateWalletTxTest.java

bullet-tooth · 2019-06-04T09:09:20Z

...demo/src/test/java/com/exonum/binding/cryptocurrency/transactions/TransferTxHistoryTest.java

  private static final KeyPair ACCOUNT_2 = PredefinedOwnerKeys.SECOND_OWNER_KEY_PAIR;

  @Test
  @RequiresNativeLibrary


The same is here. The class and tests are annotated.

bullet-tooth · 2019-06-04T09:14:01Z

exonum-java-binding/pom.xml

    <hamcrest.version>2.1</hamcrest.version>
    <junit.version>4.12</junit.version>
    <junit.jupiter.version>5.4.2</junit.jupiter.version>
+    <junit-platform-testkit.version>1.5.0-M1</junit-platform-testkit.version>


Please move it to the TestKit pom (or is it needed for other modules?)

bullet-tooth · 2019-06-04T09:18:01Z

exonum-java-binding/testkit/src/main/java/com/exonum/binding/testkit/TestKit.java

 * created.
 *
 * @see <a href="https://exonum.com/doc/version/0.11/get-started/test-service/">TestKit documentation</a>
- *      <a href="https://exonum.com/doc/version/0.11/advanced/consensus/specification/#pool-of-unconfirmed-transactions">Pool of Unconfirmed Transactions</a>


Please fix @linkplain above ⏫ (redundant } )

bullet-tooth · 2019-06-04T09:23:16Z

exonum-java-binding/testkit/src/main/java/com/exonum/binding/testkit/TestKitExtension.java

+ * given builder. Example usage:
+ *
+ * <pre><code>
+ * &#64;RegisterExtension


I see neither examples of creating TestKit in a declarative way (using @ExtendWith) or a Javadoc describing why it's not possible/forbidden.

It's not possible to use @ExtendWith because that way we can't pass a TestKit builder as an argument to extension constructor. Should I specify that in Javadoc?

Since we provide examples of usage right in the Javaddocs (and, later, on the web), I don't think it is necessary to say that "You can't register it with ExtendWith". If users try and it does not work — we can reasonably assume they will get to the (any) docs.

dmitry-timofeev · 2019-06-04T11:14:04Z

exonum-java-binding/testkit/src/main/java/com/exonum/binding/testkit/TimeProvider.java

  ZonedDateTime getTime();
+
+  /**
+   * Returns a copy of this time provider.


I do not understand how it is supposed to work. It is apparently required when testkit is
static and it has a time provider set. In this case the time provider needs to be pre-configured
in a static context (= before @BeforeAll).

Some issues with the current design:

Does not make sense with FakeTimeProvider unless it is constant (= never changes the current time),
because the client cannot access the actual time provider used by the testkit. We can probably
make testkit return it, but I think it will only complicate things (pass one provider to the
builder, but get another one from the teskit). On the other hand, if we do not copy (in a static context),
and the client puts it in a static field and then reconfigures in tests —
they will end up with broken initial state in case of single-threaded execution of tests or concurrency
issues (and anything broken) in case of multi-threaded execution.

Cannot use mocks even without the extension because they cannot be cloned (well, can with a wrapper with a throwing copy implementation).

Unless I'm missing something, copying seems restrictive and not that easy to use or fix.
But it is required only for static extensions. What we could do:

'Fix' by adding Testkit#getTimeProvider and some obscure documentation.

Ignore the concurrency issue (i.e., static extension with a shared template builder
and a shared time provider will use the same instance in all tests). If a client breaks
it by modifying the provider in any test, it is their problem.

Forbid static extensions — we seem to have found a good reason :-)

Anything else?

I'd forbid (discourage?) static extensions. It's easier for us, will prevent any other possible issues with having a static extensions (I can't think of any right now though) and won't limit user in any way.

Also I suppose it's better to make a copy of templateTestKitBuilder on every parameter resolve anyway (as we do now), even though we discourage having a static extension. Even if someone uses static, they are unlikely to have any issues in most cases.

OK, I agree with discouraging it because apparently there is no reasonable way to support static. Shall we also "forbid" (probably, by supporting class-level callbacks, and, if they get invoked — throwing an Exception)? Are there any downsides in doing that (i.e., is it possible that a user is forced to use static but it won't cause any problems)?

...nding/integration-tests/src/test/java/com/exonum/binding/test/BlockchainIntegrationTest.java

.../integration-tests/src/test/java/com/exonum/binding/test/TimeSchemaProxyIntegrationTest.java

exonum-java-binding/testkit/pom.xml

exonum-java-binding/testkit/src/main/java/com/exonum/binding/testkit/TestKitExtension.java

bullet-tooth · 2019-06-06T07:55:50Z

I see old way usage of TestKit in com.exonum.binding.test.BlockchainIntegrationTest. Or is it in the scope of another task?

dmitry-timofeev · 2019-06-06T09:27:18Z

I see old way usage of TestKit in com.exonum.binding.test.BlockchainIntegrationTest. Or is it in the scope of another task?

It makes little sense to use the extension when you need to inject it in @BeforeEach and then use in test and utility methods. It is simpler to put it in a field, to avoid passing explicitly between methods.
Testkit, however, would automatically destroy it (no need for @AfterEach), but the extension overhead (in terms of extra lines of code to register it and some complexity) seems higher.

MakarovS added 2 commits May 24, 2019 13:45

ECR-3076 Add TestKit extension

356705b

ECR-3076 Add disposing internal

60faabd

MakarovS added the work-in-progress 👷‍♂️ Do not expect reviewers to provide any feedback on WIP PRs — please ask for it explicitly! label May 27, 2019

MakarovS requested review from dmitry-timofeev and bullet-tooth May 27, 2019 05:16

Merge remote-tracking branch 'remotes/origin/master' into ECR-3076

c45e851

dmitry-timofeev reviewed May 27, 2019

View reviewed changes

ECR-3076 Add annotations and review fixes

c5f810d

dmitry-timofeev reviewed May 29, 2019

View reviewed changes

dmitry-timofeev reviewed May 30, 2019

View reviewed changes

MakarovS added 2 commits June 3, 2019 02:47

ECR-3076 Review fixes and tests

164948c

Merge remote-tracking branch 'remotes/origin/master' into ECR-3076

447cbf8

dmitry-timofeev reviewed Jun 3, 2019

View reviewed changes

MakarovS added 4 commits June 3, 2019 23:37

ECR-3076 Fix tests and review comments

b78615b

Merge remote-tracking branch 'remotes/origin/master' into ECR-3076

473ca29

ECR-3076 Checkstyle fix

9266f6d

ECR-3076 Use TestKit extension in TestKit tests

280fd7f

MakarovS removed the work-in-progress 👷‍♂️ Do not expect reviewers to provide any feedback on WIP PRs — please ask for it explicitly! label Jun 4, 2019

bullet-tooth reviewed Jun 4, 2019

View reviewed changes

dmitry-timofeev suggested changes Jun 4, 2019

View reviewed changes

MakarovS added 2 commits June 4, 2019 22:16

ECR-3076 Review fixes

2150069

Merge remote-tracking branch 'remotes/origin/master' into ECR-3076

f5baa27

dmitry-timofeev approved these changes Jun 5, 2019

View reviewed changes

Merge remote-tracking branch 'remotes/origin/master' into ECR-3076

9164ddd

dmitry-timofeev merged commit 62144ec into master Jun 6, 2019

dmitry-timofeev deleted the ECR-3076 branch June 6, 2019 09:28

MakarovS mentioned this pull request Jul 2, 2019

Add test extension [ECR-2634] #582

Closed

7 tasks

Add TestKit JUnit extension [ECR-3076] #913

Add TestKit JUnit extension [ECR-3076] #913

Uh oh!

Conversation

MakarovS commented May 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Overview

Definition of Done

Uh oh!

coveralls commented May 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MakarovS commented May 27, 2019 •

edited

Loading

coveralls commented May 27, 2019 •

edited

Loading