-
Notifications
You must be signed in to change notification settings - Fork 3
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
test: Add E2E test and enhance simulator flexibility #330
Conversation
f0361e4
to
5eff4f5
Compare
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 is my first review, I will need to do another one for sure. Generally looks good, but I have a couple of questions and a couple of concerns, also a few nit picks.
General comment:
Can you please consider to add javadocs to the tests describing what they aim to achieve? Will make them a lot easier to review and for us to know their purpose in order to validate them?
simulator/src/main/java/com/hedera/block/simulator/config/data/StreamStatus.java
Outdated
Show resolved
Hide resolved
simulator/src/main/java/com/hedera/block/simulator/grpc/PublishStreamGrpcClientImpl.java
Outdated
Show resolved
Hide resolved
simulator/src/main/java/com/hedera/block/simulator/config/data/StreamStatus.java
Outdated
Show resolved
Hide resolved
simulator/src/main/java/com/hedera/block/simulator/grpc/PublishStreamGrpcClientImpl.java
Show resolved
Hide resolved
simulator/src/test/java/com/hedera/block/simulator/BlockStreamSimulatorTest.java
Outdated
Show resolved
Hide resolved
...src/main/java/com/hedera/block/suites/persistence/positive/PositiveDataPersistenceTests.java
Show resolved
Hide resolved
suites/src/main/java/com/hedera/block/suites/grpc/positive/PositiveEndpointBehaviourTests.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.
Nice work, I like seeing all this new test coverage! I added one suggestion.
simulator/src/main/java/com/hedera/block/simulator/grpc/PublishStreamGrpcClientImpl.java
Outdated
Show resolved
Hide resolved
Signed-off-by: georgi-l95 <[email protected]> feat: add stream status object Signed-off-by: georgi-l95 <[email protected]> chore: refactor and refine Signed-off-by: georgi-l95 <[email protected]> feat: additional refactor Signed-off-by: georgi-l95 <[email protected]> test: fix unit tests Signed-off-by: georgi-l95 <[email protected]>
Signed-off-by: georgi-l95 <[email protected]> test: fix unit test Signed-off-by: georgi-l95 <[email protected]> chore: cleanup Signed-off-by: georgi-l95 <[email protected]> test: unit test Signed-off-by: georgi-l95 <[email protected]> test: unit tests Signed-off-by: georgi-l95 <[email protected]> address feedback Signed-off-by: georgi-l95 <[email protected]>
Signed-off-by: georgi-l95 <[email protected]>
Signed-off-by: georgi-l95 <[email protected]>
0f4fe61
to
6723a79
Compare
Signed-off-by: georgi-l95 <[email protected]>
Signed-off-by: georgi-l95 <[email protected]>
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.
@georgi-l95 looks good, but the thread pool requires some changes as the current implementation will still silence exceptions.
suites/src/main/java/com/hedera/block/suites/CustomThreadPoolExecutor.java
Outdated
Show resolved
Hide resolved
suites/src/main/java/com/hedera/block/suites/CustomThreadPoolExecutor.java
Outdated
Show resolved
Hide resolved
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.
Looks great! Good job @georgi-l95!
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.
Approval applies to .gitignore
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #330 +/- ##
============================================
+ Coverage 94.05% 94.62% +0.57%
- Complexity 300 316 +16
============================================
Files 68 69 +1
Lines 1177 1228 +51
Branches 82 84 +2
============================================
+ Hits 1107 1162 +55
+ Misses 57 55 -2
+ Partials 13 11 -2
|
Description:
This PR adds new functionality aimed at making the simulator a better and more flexible test driver for E2E tests. It achieves this by:
Related issue(s):
Fixes #186
Notes for reviewer:
Checklist