Skip to content

Conversation

MakarovS
Copy link
Contributor

@MakarovS MakarovS commented Jul 2, 2019

Overview

Add TestKit to BOM and archetype modules.


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

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

@MakarovS MakarovS added the work-in-progress 👷‍♂️ Do not expect reviewers to provide any feedback on WIP PRs — please ask for it explicitly! label Jul 2, 2019
<groupId>com.exonum.binding</groupId>
<artifactId>exonum-testkit</artifactId>
<version>${project.version}</version>
<scope>test</scope>
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure BOM shall specify the scope. Other test-only artifacts do not.

@dmitry-timofeev
Copy link
Contributor

The archetype project must depend on the testkit as well (as it does on core in runtime scope), as documented in the issue.

@coveralls
Copy link

coveralls commented Jul 4, 2019

Coverage Status

Coverage remained the same at 85.465% when pulling d7380e9 on ECR-3056 into 6f076b8 on master.

@MakarovS MakarovS removed the work-in-progress 👷‍♂️ Do not expect reviewers to provide any feedback on WIP PRs — please ask for it explicitly! label Jul 4, 2019
<version>${maven-archetype.version}</version>
<configuration>
<properties>
<nativeLibPath>${project.parent.basedir}/core/rust/target/debug</nativeLibPath>
Copy link
Contributor

Choose a reason for hiding this comment

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

@dip56 Would you please test that release builds continue to work (we didn't use to use the native library in archetype ITs), and the generated project works with the installed app (the current 0.7.0-SNAPSHOT)?

}

@Test
void testServiceInstantiation() {
Copy link
Contributor

@bullet-tooth bullet-tooth Jul 9, 2019

Choose a reason for hiding this comment

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

wouldn't it be better to create a separate class with junit5 extension? Therefore we will have 2 separate tests:

  1. Check that the module is properly configured
  2. Check that exonum-app is properly installed

Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure. This particular test is only useful at project initialization to verify the app is installed properly + as an example of testkit configuration 🤔 We can extend some comments here and there/modify the test display name to make their purpose more apparent.

Copy link
Contributor

Choose a reason for hiding this comment

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

I removed the existing tests as they do not bring much value on top of this new one.

Copy link
Contributor

Choose a reason for hiding this comment

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

and documented the likely failure

@dmitry-timofeev dmitry-timofeev merged commit 40b5e54 into master Jul 10, 2019
@dmitry-timofeev dmitry-timofeev deleted the ECR-3056 branch July 10, 2019 14:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants