Migrate common module to Junit5 #1135
Conversation
|
This PR solve some of the test cases based on the Issue #1103 |
|
cc @bashir2 |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1135 +/- ##
============================================
- Coverage 51.76% 51.67% -0.09%
+ Complexity 669 668 -1
============================================
Files 95 95
Lines 5612 5612
Branches 731 731
============================================
- Hits 2905 2900 -5
- Misses 2425 2430 +5
Partials 282 282 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Thanks @meetmehta1198 for the changes and apologies for the delay in the review. There are few comments, can you please have a look.
pipelines/common/src/test/java/com/google/fhir/analytics/FhirStoreUtilTest.java
Outdated
Show resolved
Hide resolved
pipelines/common/src/test/java/com/google/fhir/analytics/FhirStoreUtilTest.java
Show resolved
Hide resolved
meetmehta1198
force-pushed
the
common-module-junit5
branch
2 times, most recently
from
4145b99 to
7b0ecdd
Compare
meetmehta1198
force-pushed
the
common-module-junit5
branch
from
7b0ecdd to
e22308e
Compare
|
@chandrashekar-s can you please review this PR |
There was a problem hiding this comment.
Thanks @meetmehta1198 for these changes. I am extremely sorry for delaying the review for this PR as I was involved in some activities of another project. The changes look good, there is only one minor comment. Can you please have a look at that?
meetmehta1198
force-pushed
the
common-module-junit5
branch
2 times, most recently
from
a709fc7 to
bc37de5
Compare
|
Hi @meetmehta1198 for the changes, there is an build error which you can see from the logs. Can you please have a look at it. |
|
Hi @meetmehta1198, just a reminder to look into the build issue. Please let me know if you face any issues while accessing the build logs. |
|
I will take a look into it! Thanks |
meetmehta1198
force-pushed
the
common-module-junit5
branch
from
b8519ef to
4dee999
Compare
|
@chandrashekar-s I have updated the code and the build is successful. Can you please approve this PR? |
meetmehta1198
changed the title
There was a problem hiding this comment.
Thanks @meetmehta1198 for the changes, they all look good now. You can go ahead and merge.
|
Thanks @chandrashekar-s for all the feedback and approval. Great experience working with you! |
I do not have necessary permissions to merge code! |
|
Thanks @meetmehta1198 for this and congratulations on your first contribution to this repo. Just one minor comment: It seems to me that from #1103 you are fixing tests under the |
I have updated the PR description, so that it does not close the main issue. |
Description of what I changed
This PR migrates the
commonmodule to Junit5 and addresses a part of the issue: #1103E2E test
TESTED:
Please replace this with a description of how you tested your PR beyond the
automated e2e/unit tests.
Checklist: I completed these to help reviewers :)
I have read and will follow the review process.
I am familiar with Google Style Guides for the language I have coded in.
No? Please take some time and review Java and Python style guides.
My IDE is configured to follow the Google code styles.
No? Unsure? -> configure your IDE.
I have added tests to cover my changes. (If you refactored existing code that was well tested you do not have to add tests)
I ran
mvn clean packageright before creating this pull request and added all formatting changes to my commit.All new and existing tests passed.
My pull request is based on the latest changes of the master branch.
No? Unsure? -> execute command
git pull --rebase upstream master