Skip to content
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.

Sign up for GitHub

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

Jump to bottom

testing: Utils test implemented in core module #420

Closed
wants to merge 3 commits into from
Closed

testing: Utils test implemented in core module #420

wants to merge 3 commits into from

Conversation

Ana2k
Copy link
Contributor

@Ana2k Ana2k commented Apr 2, 2021

IMPORTANT: All PRs must be linked to an issue (except for extremely trivial and straighforward changes).

Fixes #380

Description
Added UtilTest.kt file as unit test for Util.kt

Alternative(s) considered
N/A

Type
Choose one: Testing

Screenshots (if applicable)
N/A

Checklist

  • I have read and acknowledge the Code of conduct
  • I have read How to Contribute
  • I have read the Developer's guide
  • I have signed the Google Individual CLA, or I am covered by my company's Corporate CLA
  • I have discussed my proposed solution with code owners in the linked issue(s) and we have agreed upon the general approach
  • I have run ./gradlew spotlessApply and ./gradlew spotlessCheck to check my code follows the style guide of this project
  • I have run ./gradlew check and ./gradlew connectedCheck to test my changes locally
  • I have built and run the reference app(s) to verify my change fixes the issue and/or does not break the reference app(s)

@Ana2k
Copy link
Contributor Author

Ana2k commented Apr 2, 2021 •
edited
Loading

@jingtang10 @deepankarb i have implemented this testing file. Please review this pr. Thanks!

epicadk
epicadk reviewed Apr 3, 2021
View reviewed changes
core/src/test/java/com/google/android/fhir/UtilTest.kt Outdated
import android.os.Build
import java.time.ZoneId
import java.time.format.DateTimeFormatter
import java.util.*
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Spotless check is failing most likely because of this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks so much for the review @epicadk ! I have updated the changes, hope the build runs now. :)

epicadk
epicadk reviewed Apr 4, 2021
View reviewed changes
core/src/test/java/com/google/android/fhir/UtilTest.kt Outdated
@@ -1,5 +1,5 @@
/*
* Copyright 2021 Google LLC
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this change?

@Ana2k Ana2k changed the title Utils test testing: Utils test implemented in core module Apr 4, 2021
jingtang10
jingtang10 requested changes Apr 13, 2021
View reviewed changes
Copy link
Collaborator

@jingtang10 jingtang10 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks!

core/src/test/java/com/google/android/fhir/UtilTest.kt Outdated
DateTimeFormatter.ofPattern("yyyy-MM-dd'T'HH:mm:ss.SSSXXX", Locale.getDefault())
.withZone(ZoneId.systemDefault())
val expectedResult = simpleDateFormat.format(currentDate.toInstant())
assertEquals(currentDate.toTimeZoneString(), expectedResult)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We use Truth: https://truth.dev/

core/src/test/java/com/google/android/fhir/UtilTest.kt Outdated
@Config(sdk = [Build.VERSION_CODES.P])
class UtilTest {
@Test
fun checkTimeZoneFormat() {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
fun checkTimeZoneFormat() {
fun shouldFormatDate() {

core/src/test/java/com/google/android/fhir/UtilTest.kt Outdated
class UtilTest {
@Test
fun checkTimeZoneFormat() {
val currentDate = Date()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd rather use a fixed value in test to avoid flakiness.

@jingtang10
Copy link
Collaborator

Is this still being worked on?

@Ana2k
Copy link
Contributor Author

Ana2k commented May 8, 2021

Is this still being worked on?

Hi @jingtang10 I apologise for the delay. Sadly I had COVID for quite a while. I have just fully recovered.
Really excited to start contributing again.
Will get this done ASAP.

@jingtang10
Copy link
Collaborator

Is this still being worked on?

Hi @jingtang10 I apologise for the delay. Sadly I had COVID for quite a while. I have just fully recovered.
Really excited to start contributing again.
Will get this done ASAP.

Hi @Ana2k - sorry to hear that you had covid but really pleased to hear that you're fully recovered. Please take it easy! Kind regards and best wishes to you!

@Ana2k Ana2k requested a review from aditya-07 as a code owner June 2, 2021 12:10
@Ana2k
Copy link
Contributor Author

Ana2k commented Jun 2, 2021 •
edited
Loading

@jingtang10 theres some updates in the Util.kt file and its test file in the master branch.
The tests wrt Time and date were absent in pre existing UtilTest.kt.
So, I am proceeding with updating time and date tests to it.
Please let me know if further changes are required in that file.

@Ana2k
Copy link
Contributor Author

Ana2k commented Jun 5, 2021

@jingtang10 theres some updates in the Util.kt file and its test file in the master branch.
The tests wrt Time and date were absent in pre existing UtilTest.kt.
So, I am proceeding with updating time and date tests to it.
Please let me know if further changes are required in that file.

@jingtang10 I have updated the shouldFormatDate function in utilTest.kt Kindly please review. Thanks!

jingtang10
jingtang10 requested changes Jun 24, 2021
View reviewed changes
engine/src/test/java/com/google/android/fhir/UtilTest.kt
DateTimeFormatter.ofPattern("yyyy-MM-dd'T'HH:mm:ssXXXXX", Locale.getDefault())
.withZone(ZoneId.systemDefault())
val expectedResult = simpleDateFormat.format(parsedDate.toInstant())
assertThat(parsedDate.toString()).isEqualTo(expectedResult)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what are you testing here? I thought you wanted to test https://github.com/google/android-fhir/blob/master/engine/src/main/java/com/google/android/fhir/Util.kt#L30?

Copy link
Contributor Author

@Ana2k Ana2k Jul 3, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, these are the changes for the same.
The tests passed on my end.
Let me know if any issues exist or more changes are required.
@maimoonak tagging you to update about previous PR.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not see the Date.toTimeZoneString method being called here. How is it being tested?

@Ana2k Ana2k requested a review from stevenckngaa as a code owner July 3, 2021 08:50
jingtang10
jingtang10 requested changes Jul 7, 2021
View reviewed changes
engine/src/test/java/com/google/android/fhir/UtilTest.kt
DateTimeFormatter.ofPattern("yyyy-MM-dd'T'HH:mm:ssXXXXX", Locale.getDefault())
.withZone(ZoneId.systemDefault())
val expectedResult = simpleDateFormat.format(parsedDate.toInstant())
assertThat(parsedDate.toString()).isEqualTo(expectedResult)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not see the Date.toTimeZoneString method being called here. How is it being tested?

@Tarun-Bhardwaj
Copy link

Hi @Ana2k , is there an ETA you can provide for completing the changes as per the review comments in order to close this PR?

@Ana2k
Copy link
Contributor Author

Ana2k commented Aug 5, 2021

Hi @Ana2k , is there an ETA you can provide for completing the changes as per the review comments in order to close this PR?

Hi @Tarun-Bhardwaj I am have almost completed working on this, ETA is around 11th August

@Tarun-Bhardwaj Tarun-Bhardwaj added this to In progress in FHIR engine library via automation Sep 20, 2021
@jingtang10
Copy link
Collaborator

jingtang10 commented Nov 10, 2021 •
edited
Loading

Closing this since it's not been worked on.

@jingtang10 jingtang10 closed this Nov 10, 2021
FHIR engine library automation moved this from In progress to Done Nov 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@epicadk epicadk epicadk left review comments

@jingtang10 jingtang10 jingtang10 requested changes

@deepankarb deepankarb Awaiting requested review from deepankarb

@kevinmost kevinmost Awaiting requested review from kevinmost

@pankajkatoch pankajkatoch Awaiting requested review from pankajkatoch

@yigit yigit Awaiting requested review from yigit

@aditya-07 aditya-07 Awaiting requested review from aditya-07 aditya-07 is a code owner

@stevenckngaa stevenckngaa Awaiting requested review from stevenckngaa

Assignees

@Ana2k Ana2k

Labels
None yet
Projects
Android FHIR SDK
Archived in project
FHIR engine library
  
Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add unit test for the Util.kt file in the core module
4 participants
@Ana2k @jingtang10 @Tarun-Bhardwaj @epicadk