Skip to content

[AC-614] : Unit test are now working#637

Merged
f4ww4z merged 9 commits intoopenmrs:masterfrom
rishabh-997:experiment
Dec 30, 2019
Merged

[AC-614] : Unit test are now working#637
f4ww4z merged 9 commits intoopenmrs:masterfrom
rishabh-997:experiment

Conversation

@rishabh-997
Copy link
Collaborator

@rishabh-997 rishabh-997 commented Dec 18, 2019

Description of what I changed

The main problem arose because of the migration of android to androidx, to which ActiveAndroid class was not up to date. It had a non-editable caches.java file which was importing LruCache.java from the android.support.v4.util package but originally, this file is found in android.util package.

I started creating custom classes for the ActiveAndroid package recursively but I almost had to re-copy the entire ActiveAndroid library, so I removed the dependency and added code for the entire ActiveAndroid library. The size of the app will have no effect as new java files are compensated by removed dependency

Issue I worked on

JIRA Issue: https://issues.openmrs.org/browse/AC-614

Checklist: I completed these to help reviewers :)

  • My pull request only contains ONE single commit
    (the number above, next to the 'Commits' tab is 1).
  • I have added tests to cover my changes. (If you refactored
    existing code that was well tested you do not have to add tests)
  • All new and existing tests passed.
  • My pull request is based on the latest changes of the master branch.

@rishabh-997
Copy link
Collaborator Author

rishabh-997 commented Dec 18, 2019

Also, please suggest any suitable alternative in case you have in mind. The reason behind this problem has been stated in description @f4ww4z

@rishabh-997 rishabh-997 mentioned this pull request Dec 20, 2019
4 tasks
@rishabh-997
Copy link
Collaborator Author

@f4ww4z sir, one more file needs to be customized in this way which I'll do tomorrow to pass travis CI, do you think this would be correct fix or something else needs to be done too ?

@f4ww4z
Copy link
Collaborator

f4ww4z commented Dec 23, 2019

@rishabh-997 Yes you need to fix the failing test at https://travis-ci.org/openmrs/openmrs-contrib-android-client/builds/626888550#L867 . Make sure the test works locally first before committing.

@f4ww4z
Copy link
Collaborator

f4ww4z commented Dec 23, 2019

Also @rishabh-997 , please claim this task in GCI dashboard first.

@rishabh-997
Copy link
Collaborator Author

Also @rishabh-997 , please claim this task in GCI dashboard first.

sir actually mobile internet has been taken down for some days because of riots going on here, can be little late, sorry for the delay.... will try to finish it up as soon as mobile networks are restored permanently

@codecov-io
Copy link

codecov-io commented Dec 24, 2019

Codecov Report

❗ No coverage uploaded for pull request base (master@8cd532b). Click here to learn what that means.
The diff coverage is 0.66%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #637   +/-   ##
=========================================
  Coverage          ?   12.61%           
=========================================
  Files             ?      231           
  Lines             ?    10610           
  Branches          ?      992           
=========================================
  Hits              ?     1338           
  Misses            ?     9191           
  Partials          ?       81
Impacted Files Coverage Δ
.../java/org/openmrs/mobile/api/EncounterService.java 0% <ø> (ø)
...main/java/org/openmrs/mobile/dao/EncounterDAO.java 0.7% <ø> (ø)
...g/openmrs/mobile/utilities/ResourceSerializer.java 50% <ø> (ø)
...main/java/org/openmrs/mobile/models/Obscreate.java 0% <ø> (ø)
...n/java/org/openmrs/mobile/api/FormListService.java 0% <ø> (ø)
...n/java/org/openmrs/mobile/models/FormResource.java 57.69% <ø> (ø)
.../java/org/openmrs/mobile/models/EncounterType.java 71.42% <ø> (ø)
...ava/org/openmrs/mobile/models/Encountercreate.java 0% <ø> (ø)
...enmrs/mobile/api/repository/PatientRepository.java 13.11% <ø> (ø)
.../src/main/java/org/openmrs/mobile/models/Link.java 0% <ø> (ø)
... and 31 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8cd532b...e5616bc. Read the comment docs.

@rishabh-997
Copy link
Collaborator Author

rishabh-997 commented Dec 24, 2019

@f4ww4z here is one thing, I had no idea regarding this GCI thing, so I worked on it and resolved the issue, but I am not in GCI. I have worked hard to resolve this issue so can you review it, ill take care next time...

passed all 104 tests

@rishabh-997 rishabh-997 changed the title [AC-635] : Unit test are now working [AC-614] : Unit test are now working Dec 24, 2019
Copy link
Collaborator

@f4ww4z f4ww4z left a comment

Choose a reason for hiding this comment

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

Appreciated the effort @rishabh-997 ! See my comments below.

Do we really need to implement all of the serializers? e.g. BigDecimalSerializer, CalendarSerializer, etc.

Make sure your format your changes using AS's default formatter.

@@ -0,0 +1,29 @@
package org.openmrs.mobile.utilities.ActiveAndroid.serializer;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Add the OpenMRS license header. Do we need this serializer?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we need this serializer? @rishabh-997

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

actually, there were few files of active android which need not be created, but in order to remove the dependency completely from gradle, i copied all of them so there might not be any duplicates and app size wont increase... ill check , if these serializers are going unused, ill remove the file

@rishabh-997
Copy link
Collaborator Author

@f4ww4z I have made the changes you requested, if there is no problem with the PR, then ill squash the commits

@f4ww4z f4ww4z mentioned this pull request Dec 28, 2019
4 tasks
Copy link
Collaborator

@f4ww4z f4ww4z left a comment

Choose a reason for hiding this comment

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

See my comments below.

Do we really need to implement all of the serializers? e.g. BigDecimalSerializer, CalendarSerializer, etc.

@rishabh-997

@Override
public void onCreate() {
super.onCreate();
ActiveAndroid.initialize(this);
Copy link
Collaborator

Choose a reason for hiding this comment

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

@Override
public void onTerminate() {
super.onTerminate();
ActiveAndroid.dispose();
Copy link
Collaborator

Choose a reason for hiding this comment

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

this also.

}

//////////////////////////////////////////////////////////////////////////////////////
// PUBLIC METHODS
Copy link
Collaborator

Choose a reason for hiding this comment

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

This divider was already declared before.

@@ -0,0 +1,29 @@
package org.openmrs.mobile.utilities.ActiveAndroid.serializer;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we need this serializer? @rishabh-997

@rishabh-997 rishabh-997 requested a review from f4ww4z December 28, 2019 13:41
Copy link
Collaborator

@f4ww4z f4ww4z left a comment

Choose a reason for hiding this comment

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

Looks good @rishabh-997 !

@f4ww4z f4ww4z merged commit 21ee22d into openmrs:master Dec 30, 2019
f4ww4z pushed a commit that referenced this pull request Dec 31, 2019
rishabh-997 added a commit to rishabh-997/openmrs-contrib-android-client that referenced this pull request Jan 5, 2020
@rishabh-997 rishabh-997 deleted the experiment branch March 27, 2020 11:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants