[ETCM-126] split JsonRpcControllerSpec #734
Conversation
…r and simplify tests using them
…erSpec into JsonRpcControllerPersonalSpec
…ec to JsonRpcCOntrollerEthTransactionSpec
mmrozek
left a comment
mmrozek
left a comment
There was a problem hiding this comment.
Nice job. One doubt only
| val version = Config.clientVersion | ||
| val blockGenerator = mock[EthashBlockGenerator] | ||
|
|
||
| override implicit lazy val system = ActorSystem("JsonRpcControllerSpec_System") |
There was a problem hiding this comment.
Do we need to create ActorSystem for each test?
There was a problem hiding this comment.
This.
I'd say it's preferrable to use akka's TestKit on suite level and use there WithActorSystemShutdown trait.
Benefits are as follows:
- Creating and shutting down systems is quite expensive operation.
- Actors of any kind are created per spec anyways so lack of isolation is not an issue I think
- In other project we had real problems with tests due to leaking running actor systems
There was a problem hiding this comment.
How about merging this PR and fix the unnecessary spawning actor system in a separate one.
There is a pretty complex wiring using cake pattern (actor system that comes from ActorSystemBuilder (via StdTestConsensusBuilder via ScenarioSetup via EphemBlockchainTestSetup and ActorSystemBilder is required in StdLedgerBuilder via self type). I need some time to figure out how to do this.
There was a problem hiding this comment.
Sounds good to me. And such task/PR could be extended to making sure actor system is properly closed in all tests.
Description
JsonRpcControllerSpec file contained around 2000 lines of code. This is hard to maintain. Context for this PR: #726 (comment)
Proposed Solution
Split it into tests related to:
JsonRpcControllerPersonalSpecpersonal_XYZJsonRpcControllerEthTransactionSpecrelated to eth transaction requestsJsonRpcControllerEthSpecother eth requestsJsonRpcControllerSpecother requestsBefore split there were utility function extracted that simplify creating requests and creating JsonRpcService:
newJsonRpcController,newJsonRpcRequestin 53283b7Important Changes Introduced
Testing