-
Notifications
You must be signed in to change notification settings - Fork 2
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.
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
refactor: simulator entry point to be flexible as test driver #226
refactor: simulator entry point to be flexible as test driver #226
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a blocker, but please consider to use the final
keyword on all places including local variables (inside methods) and method parameters as well. It is best that we are as strict as possible by default. One of the best reasons to use final
for parameters and local vars is so that we do not accidentally change the reference, which could lead to silent bugs. With final
if we try to change the ref, we get a compilation error.
simulator/src/main/java/com/hedera/block/simulator/BlockStreamSimulatorApp.java
Outdated
Show resolved
Hide resolved
simulator/src/main/java/com/hedera/block/simulator/BlockStreamSimulatorApp.java
Show resolved
Hide resolved
simulator/src/main/java/com/hedera/block/simulator/BlockStreamSimulatorApp.java
Outdated
Show resolved
Hide resolved
Signed-off-by: georgi-l95 <[email protected]> spotless Signed-off-by: georgi-l95 <[email protected]>
8b1015c
to
e780607
Compare
Signed-off-by: georgi-l95 <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Resolved for me.
simulator/src/main/java/com/hedera/block/simulator/BlockStreamSimulatorApp.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have some questions about DI versus new constructors
simulator/src/main/java/com/hedera/block/simulator/BlockStreamSimulatorApp.java
Show resolved
Hide resolved
simulator/src/main/java/com/hedera/block/simulator/BlockStreamSimulatorApp.java
Show resolved
Hide resolved
simulator/src/test/java/com/hedera/block/simulator/BlockStreamSimulatorTest.java
Show resolved
Hide resolved
@@ -92,10 +92,24 @@ void start_exitByBlockNull() throws InterruptedException, ParseException, IOExce | |||
assertTrue(blockStreamSimulator.isRunning()); | |||
} | |||
|
|||
void start_usingConfigurationConstructor() | |||
throws InterruptedException, ParseException, IOException { | |||
blockStreamSimulator = new BlockStreamSimulatorApp(configuration); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps use local variables rather than member variables to ensure deterministic behavior here. Like changing to final BlockStreamSimulatorApp() blockStreamSimulator = new BlockStreamSimulatorApp(configuration);
private static Configuration loadDefaultConfiguration() throws IOException { | ||
ConfigurationBuilder configurationBuilder = | ||
ConfigurationBuilder.create() | ||
.withSource(SystemEnvironmentConfigSource.getInstance()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code looks like it's from Server.java
. We're going to change how we bring in environment variables. Perhaps coordinate with Atanas to make the same changes here
void start_usingEmptyConstructor() throws IOException, InterruptedException, ParseException { | ||
blockStreamSimulator = new BlockStreamSimulatorApp(); | ||
blockStreamSimulator.start(); | ||
assertTrue(blockStreamSimulator.isRunning()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we can expand these tests using mocks or other techniques to check that get default configuration is actually present and correct, that overriding properties and env variables result in expected values, test the stop()
method, etc
Closing in favor of #242 |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #226 +/- ##
=========================================
Coverage 99.66% 99.67%
- Complexity 247 252 +5
=========================================
Files 50 50
Lines 903 917 +14
Branches 61 61
=========================================
+ Hits 900 914 +14
Misses 3 3
|
Description:
This PR aims to refactor the simulator entry point and add flexabilty, in terms of how it can be started.
Adding two new constructors and moving the dagger initialization in the applicaiton itself, rather being in the main method.
This allows us to use it easy in the suites module, where we have the E2E tests.
Related issue(s):
Fixes #190
Notes for reviewer:
Checklist