Skip to content
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

Jump to bottom

Improve the documentation of a test. #284

Merged
merged 3 commits into from
Jun 8, 2018
Merged

Improve the documentation of a test. #284

merged 3 commits into from
Jun 8, 2018

Conversation

dmitry-timofeev
Copy link
Contributor

@dmitry-timofeev dmitry-timofeev commented Jun 7, 2018 •
edited
Loading

Overview

Improve the documentation of a test, tone down the language.

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

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

MakarovS
MakarovS previously approved these changes Jun 7, 2018
View reviewed changes
Copy link
Contributor

@MakarovS MakarovS left a comment •
edited
Loading

Choose a reason for hiding this comment

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

👍
It's a minor issue, but I suggest we decide on whether or not we should put a period in the end of a comment and stick to it :) I propose we either always do it or omit it when there's only one sentence in a comment (I think the first way is more consistent though)

@dmitry-timofeev
Copy link
Contributor Author

It's a minor issue, but I suggest we decide on whether or not we should put a period in the end of a comment and stick to it :)

That's a good question. Sometimes a period feels better (when there is a proper sentence), sometimes a colon, sometimes — nothing :-) I'd strive for local consistency. I wonder if we shall require a period or no period — do you think it must be a strict rule?

@dmitry-timofeev dmitry-timofeev merged commit 7f8a2e8 into exonum:master Jun 8, 2018
@dmitry-timofeev dmitry-timofeev deleted the improve-documentation-in-vertx-test branch June 8, 2018 15:52
@MakarovS
Copy link
Contributor

MakarovS commented Jun 8, 2018

That's a good question. Sometimes a period feels better (when there is a proper sentence), sometimes a colon, sometimes — nothing :-) I'd strive for local consistency. I wonder if we shall require a period or no period — do you think it must be a strict rule?

No, I'm not a proponent of strict rules for such trifles :) Short comments feel better without periods indeed, though for longer sentences I would put periods. Let's leave it as it is for now.

@stanislav-tkach
Copy link
Contributor

Personally, I prefer "strict rules" (and dots at the end of the comments). 🙃

vitvakatu pushed a commit that referenced this pull request Jun 19, 2018
Update QA service to latest master (#310)
8c266c6 
* Add third party licenses for direct Rust dependencies [ECR-1502]. (#277)

* Update the project dependencies [ECR-1521, ECR-1518]:

- Add versions plugin
- Update the plugins that we use, specify the versions
  in the pluginManagement.
- Update the versions of dependencies.
- Disable Powermock tests until the library is updated.
- Verify the project builds on 10, run tests with openjdk-10 on the CI server.
- Check for updates on the CI server.

* Use the default Java compiler instead of error-prone:

Error-prone does not support Java 10 yet. A setup to activate
it conditionally would be too verbose (a profile activation
in each sub-module, because it does not work for
<build><pluginManagement>...).

See google/error-prone#860

* Do not install RocksDb — prevents SIGILL on older systems.

On some systems with older CPUs our build produces an executable
with an illegal instruction for those processors.

The problem needs further investigation, but if we do not use
the system version of RocksDb, it goes away. As I remember
(have no stack at the moment), the illegal instruction appears
somewhere in RocksDb initialization.

See: ECR-1558

* Upgrade to Exonum 0.8 [ECR-1620]

* Improve the documentation of a test. (#284)

* Change Java serialization to protobuf and use owner’s public key as wallet id [ECR-1493, ECR-1494]

  - Change Java Serialization of transactions and wallets to protobuf.
  - Wallet String name is changed to the  wallet owner’s public key.
  - Fix a bug in send money transaction when a transaction with the sender 
  equal to the receiver resulted in doubling the balance of that wallet.

PR: #272

* Messages payload length fix [ECR-1624]:

'payload length' field in the message header used to be equal to 
'body' length in bytes, while it should be equal to the size 
of the whole message (header + body + signature).

Also, improve the tests and exception messages.

PR: #285

* Fix the service API path: (#288)

The service API path must start with a slash 
to be correctly mounted.

* Fix the transaction location URL [ECR-1648] (#293).

The base path of explorer was changed in Exonum 0.7 
from ""/api/system/v1" to "/api/explorer/v1/".

* Add Javadoc plugin [ECR-1510] (#290):

Javadoc generation does not happen during normal builds 
and must be triggered manually. JARs with the Javadoc 
for all modules can be generated with 

$ mvn javadoc:jar

* Fix build on Travis CI [ECR-1658] (#292):

* Build the dependencies instead of using installed in the system
 to prevent illegal instructions appearing in our artefacts (the actual cause
 is still unknown).
* Disable caching of rust/target for it takes at least 2.5G, 
which is too much for Travis to archive and upload.

* Add an Exonum App [ECR-869, ECR-867] (#238):

Implemented JavaServiceRuntime abstraction around JVM 
to configure and run Java services. 

JavaServiceRuntime is a ServiceFactory, allowing to configure
Java services in the same way as any other Exonum service.

Also includes EJB App - a Rust application to configure and run
an Exonum node with a specific Java service.

* Add LICENSE and license headers in every source file [ECR-1499] (#295):

Also added a script generating a text file with third-party licenses 
of Rust dependencies:

./generate_licenses.sh

* Prepare the build configuration for a release [ECR-1526] (#298):

Add required plugins to prepare artefacts for a publication 
to the repository.

* Update native-proxies branch to Exonum 0.8.1 [ECR-1157] (#299).

* Fix NodeProxy so that it can create a View

* Remove 'provided' scope of the system dependencies of a service: (#301)

Until we ship an app that includes these dependencies,
they must be available on the classpath of the service (the only
configuration option that we will have at 0.1 release).

* Remove unused imports.

* Use the Java home that Maven reports in the scripts setting LD_LIBRARY_PATH

* Fix the Exception class that we use in tests:

The mocked methods can not throw checked exceptions.

* Skip all tests until the bug with loading shared libraries is fixed.

* Add a tutorial describing how to launch a node with a Java service (#300)

* Fix the application name and the script to start the crypto demo.

* Update the project documents [ECR-486, ECR-1515, ECR-1516] (#296):

Add a contribution guide, clean up the readme, add a roadmap.

* Fix the formatting.

* Fix anchors in the contribution guide.

* Change travis-ci.com to travis-ci.org (#305)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MakarovS MakarovS MakarovS approved these changes

@stanislav-tkach stanislav-tkach stanislav-tkach approved these changes

@diminio1 diminio1 Awaiting requested review from diminio1

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@dmitry-timofeev @MakarovS @stanislav-tkach