Move gRPC boilerplate from :core to :daemon #4417
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ApiTestConfig works like :common Config, but specific to this subproject. BisqAppConfig is an enumeration specifying Bisq desktop and daemon options for running seednode, arbnode, bob & alices nodes in regtest / full-dao mode.
* apitest.properties - config file for customizing ApiTestConfig options * logback.xml - logging config file, will override logback files found in classpath * bitcoin.conf - bitcoin-core regtest config file, overwritten during startup with correct path to blocknofity * blocknotify - bitcoin-core blocknotify config file
Finding the pid of background linux/java processes has been difficult to do from java, so a bash script is provided to simplify the task.
The apitest.linux package is for running random bash commands, running 'bitcoind -regtest', running random 'bitcoin-cli -regtest' commands, and spinning up Bisq apps such as seednode, arbnode, and bob & alice nodes. All but random bash and bitcoin-cli commands are run in the background. The bitcoin-cli response processing is crude; a more sophiticated bitcoin-core rpc interface is not in the scope of this PR.
The driver class uses an ExecutorService to submit Callable tasks for starting bitcoind and Bisq nodes as Linux background processes. By default, ApiTestMain starts background processes to support regtest/dao testing, runs a few bitcoin-cli commands, then shuts down all background processes before exiting. (Actual API test suites have not been implemented yet.) ApiTestConfig options can be used to skip tests and/or leave background processes running indefinitely.
This gradle file is 'applied' by the main build file.
Usage:
Run a full clean, build, download dao-setup.zip,
and install the zip files contents in directory
apitest/build/resources/main:
./gradlew clean build :apitest:installDaoSetup
Download (if necessary) the dao-setup.zip file
and install its contents in directory
apitest/build/resources/main (no build).
./gradlew :apitest:installDaoSetup
Unnecessary use of fully qualified name 'System.exit' due to existing static import 'java.lang.System.exit'. (line 100) Avoid throwing raw exception types. (lines 295, 302)
Avoid throwing raw exception types. Combine nested if statements.
Avoid throwing null pointer exceptions.
There is no need for this as any JavaFX based :desktop instances will be started as background linux processes.
The `berkeleyDbLibPath` option now defaults to an empty string. If not set to a berkeley lib path on the command line or the apitest.properties file, this option is ignored, and 'bitcoind' will be started without first exporting the berkeley db library path. In other words: If the bitcoind binary is dynamically linked to berkeley db libs, export the configured berkeley-db lib path before starting 'bitcoind'. If statically linked, the berkeley db lib path will not be exported. Also fixed exception msgs to show missing config file's absolute path.
SetupTask submissions for Bisq background apps seednode, arbnode, etc., would not always complete due to a blocking stderr stream handler thread.join() call. This change makes waiting on a bash process err stream optional.
ApiTestMain will run all defined tests, but we also want to run individual test suites and test cases, and they will need to run the setup tasks as well.
Added :proto to the :apitest classpath for access to grpc service stubs (to be) used in method (unit) tests. Added new GrpcStubs class to expose the grpc service stubs to method and scenario tests. The larger goal of :apitest is end to end testing, where :cli's console output is checked for correctness. This change partially addresses two other important use cases: * "method" testing -- an analog to unit testing * "scenario" testing -- an analog to narrow functional testing For example, tests in the apitest.method package will directly call grpc services, and asserts will be made on the return values instead of console output. Tests in the apitest.scenario package will check correctness for broader use cases, such as funding a wallet, encrypting then unlocking a wallet for a specific time frame, or checking error messages from the server when a "getbalance" call is made after an "unlockwallet" timeout has expired. The broader end to end tests will not use grpc stubs.
Avoid throwing raw exception types. Document empty method body.
Codacy wants comments inside an empty method.
Add super class for all test types (method, scenario, end-to-end), and an class & method level annotation for skipping tests.
These are method test cases for gRPC methods that have already been well tested by :cli/test.sh
Add comment inside empty method.
The Scaffold#tearDown() method was split into two methods. The original tearDown() now passes the background process/task array to a new shutDownAll() method. This new method loops through the tasks in a more readable way, plainly expressing the intent to log all shutdown exceptions for each process being shut down, but not throwing an exception while processes are being shut down. The new shutDownAll() method returns the first shutdown exception encountered, which in turn is passed up to the test case's @afterall method.
This commit adds a -fallbackfee=0.0002 parameter to the start 'bitcoind'
command to keep bitcoin-core v0.20.1 working as v0.19.1 does -- with the
'fallbackfee' enabled.
Bitcoin v0.20.0 contains a fix for inconsistent behaviour related
to this fallbackfee configuration. Prior to v0.20, fallbackfee
was disabled (0) by default for the mainnet chain, but enabled
(0.0002) for the testnet and regtest chains.
A test case with bitcoin-core v0.20.1 was breaking on
the bitcoin-cli 'sendtoaddress' command, which was returning an
error message instead of a tx-id:
error code: -4
error message:
Fee estimation failed. Fallbackfee is disabled. \
Wait a few blocks or enable -fallbackfee.
Bitcoin-core v0.20.0 release notes contain info about this change:
https://bitcoin.org/en/release/v0.20.0#updated-rpcs-1
The Bitcoin-core PR is bitcoin/bitcoin#16524
This change checks the system call exit status of bitcoin-cli commands, and populates a new error message accessor if the system exist status != 0.
The default bitcoind / bitcoin-cli rpcport option has been changed from 18443 to 19443, to help avoid rpcport conflicts between apitest's bitcoind instances and other bitcoind and/or bitcion-qt instances which are probably using the bitcoin-core default (regtest) rpcport 18443. However, this commit cannot include other changes for avoiding bind address:port conflicts between apitest bitcoind instances and other regtest bitcoin-core instances because bitcoinj's bind port is hardcoded in RegTestParams.java as 18444. In order to avoid bitcoin-core regtest mode bind address conflicts, you must start or restart your bitcoind or bitcoin-qt instance with a non-default bind port argument, e.g. bitcoin-qt -regtest -port=20444
A new commit was needed to force a codacy check after changes were made to codacy rules.
This commit is for forcing a codacy check. The previous change to an .md doc did not force a codacy check.
Follow codacy rule against empty blocks.
This change moves gRPC boilerplate classes from the :core.grpc pkg into a new :daemon.grpc pkg. * The :core.grpc pkg was renamed :core.api, and no longer has any dependencies on gRPC libraries. * All core service classes in the :core.api pkg are now package protected, excepting CoreApi, making CoreApi the only possible entry point for all Grpc*Service -> -Core*Service calls. * All grpc service classes in the :daemon.grpc pkg are now package protected, excepting GrpcServer; the only class depending on Grpc*Service class is GrpcServer. * gRPC dependencies were moved from the gradle.build file's :core subproject to :daemon.
Changed the access modifier of bisq.common.config.CompositeOptionSet to make visible to bisq.apitest.config.ApiTestConfig, and removed the bisq.apitest.config.CompositeOptionSet class.
This bugfix makes it possible to pass any combination of ApiTestConfig options from a test case's @BeforeAll method to the test harness.
ghubstan
referenced
this pull request
in ghubstan/bisq
Aug 24, 2020
|
@sqrrm @ripcurlx Another commit on this PR passed the codacy analysis, but the JDK12 download failed again, just like https://travis-ci.org/github/bisq-network/bisq/jobs/720652581. |
This was referenced Aug 24, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This change moves gRPC boilerplate classes from the
:core.grpcpkg into a new:daemon.grpcpkg.The
:core.grpcpkg was renamed:core.api, and no longer has any dependencies ongRPClibraries.All core service classes in the
:core.apipkg were made package protected, exceptingCoreApi, makingCoreApithe only possible entry point for allGrpc*Service-> -Core*Servicecalls.All
Grpc*Serviceclasses in the:daemon.grpcpkg were made package protected, exceptingGrpcServer; the only class depending on the Grpc*Service classes is GrpcServer.gRPC dependencies were moved from the
gradle.buildfile's:coresub-project to:daemon.No changes were made to
apitest, but this PR's branch is based on the PR 4366 branch, so PR 4366 ought to be merged before this one.