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

Remove Powermock and fix unit and functional tests #78

Merged
merged 13 commits into from
Dec 15, 2022
Merged

Remove Powermock and fix unit and functional tests #78

merged 13 commits into from
Dec 15, 2022

Conversation

prudrabhat
Copy link
Contributor

@prudrabhat prudrabhat commented Dec 12, 2022 •
edited
Loading

Description

Unit and functional test source updates have been deferred during the adoption of Core 2.0 changes to speed up the development and limit code review size. This PR adds the deferred test changes.
Test coverage is now 93%

Added and modified tests to be compatible with Core 2.0

- Remove PowerMock usages
- Upgrade Mockito to 4.5.1 to enable static mocking
- Make test logs use Log service from Core
- Fix deprecated usages fo MobileCore.dispatchEvent(..)
- Use MobileCore.registerExtensions() in tests
- Split the utility class being used for both functional and unit tests.
- Move functional test utility classes into util package
- Remove unused helper methods  (the ones that did not have references even in 1.0 code)

Out of scope for this PR

- Refactoring all test naming to be  consistent (deferred for edge squad as a backlog item)
- Refactoring functional test flow and test thread wait logic.(deferred for edge squad as a backlog item)

Related Issue

MOB-17089

Motivation and Context

Unit and functional test source updates have been deferred during the adoption of Core 2.0 changes to speed up the development and limit code review size. This PR adds the deferred test changes.

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • I have signed the Adobe Open Source CLA.
  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

addb
addb reviewed Dec 12, 2022
View reviewed changes
...y/src/androidTest/java/com/adobe/marketing/mobile/edge/identity/IdentityAndroidTestUtil.java Outdated
governing permissions and limitations under the License.
*/

package com.adobe.marketing.mobile.edge.identity;
Copy link
Contributor

Choose a reason for hiding this comment

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

I have a proposal, should we write all our new files in kotlin? This util is written already and we should not rewrite this but do you guys think it is a good idea if we start writing all the new code/files in kotlin?
@prudrabhat @emdobrin @kevinlind

Copy link
Contributor

Choose a reason for hiding this comment

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

It will help us write the files faster and also less boilerplate code especially for test helpers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

During Core 2.0 development we found that writing new code in kotlin helped a great deal in improving code flow and and the amount of code written. Configuration and Signal were re-written in kotlin entirely!

cacheung
cacheung reviewed Dec 13, 2022
View reviewed changes
code/build.gradle Outdated Show resolved Hide resolved
prudrabhat
prudrabhat commented Dec 14, 2022
View reviewed changes
...droidTest/java/com/adobe/marketing/mobile/edge/identity/util/IdentityFunctionalTestUtil.java
import org.json.JSONException;
import org.json.JSONObject;

public class IdentityFunctionalTestUtil {
Copy link
Contributor Author

@prudrabhat prudrabhat Dec 13, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

This has been moved from edge.identity to edge.identity.util.
registerEdgeIdentityExtension() and registerBothIdentityExtensions() have been removed to use new registration logic via registerExtensions() keeping the rest same.

...eidentity/src/androidTest/java/com/adobe/marketing/mobile/edge/identity/util/TestHelper.java Show resolved Hide resolved
...ty/src/androidTest/java/com/adobe/marketing/mobile/edge/identity/util/ADBCountDownLatch.java Show resolved Hide resolved
...entity/src/androidTest/java/com/adobe/marketing/mobile/edge/identity/IdentityBootUpTest.java Outdated Show resolved Hide resolved
.../src/androidTest/java/com/adobe/marketing/mobile/edge/identity/IdentityECIDHandlingTest.java Outdated Show resolved Hide resolved
...rc/androidTest/java/com/adobe/marketing/mobile/edge/identity/IdentityFunctionalTestUtil.java Outdated Show resolved Hide resolved
.../edgeidentity/src/test/java/com/adobe/marketing/mobile/edge/identity/IdentityStateTests.java Outdated Show resolved Hide resolved
addb
addb reviewed Dec 14, 2022
View reviewed changes
.../src/androidTest/java/com/adobe/marketing/mobile/edge/identity/IdentityECIDHandlingTest.java
Arrays.asList(
MonitorExtension.EXTENSION,
Identity.EXTENSION,
com.adobe.marketing.mobile.Identity.EXTENSION
Copy link
Contributor

Choose a reason for hiding this comment

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

I see this test is failing. We might need to check what is going on here.

Copy link
Contributor Author

@prudrabhat prudrabhat Dec 14, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Ah. Looks like I missed adding a note for this in the description. This is a known regression (the same one that was discussed on slack) that we identified in Identity 2.0 extension (this is being fixed). I did verify that the tests pass with the patch fix that was made to Identity 2.0 . So once the fix is merged, a rerun is sufficient for this test.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see 👍. I missed this discussion.

Copy link
Contributor

Choose a reason for hiding this comment

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

@cacheung The test failure will be addressed later.

addb
addb approved these changes Dec 14, 2022
View reviewed changes
Copy link
Contributor

@addb addb left a comment

Choose a reason for hiding this comment

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

lgtm!

@cacheung
Copy link
Contributor

For this deepCopy
https://github.com/prudrabhat/aepsdk-edgeidentity-android/blob/dev-v2.0.0_unit_functional_tests/code/edgeidentity/src/main/java/com/adobe/marketing/mobile/edge/identity/Utils.java#L61

Can we use Core's?
https://github.com/adobe/aepsdk-core-android/blob/dev-v2.0.0/code/android-core-library/src/main/java/com/adobe/marketing/mobile/util/EventDataUtils.java#L137

@cacheung
Copy link
Contributor

We don't need sourceSets in build.gradle now after the restructure.
https://github.com/prudrabhat/aepsdk-edgeidentity-android/blob/dev-v2.0.0_unit_functional_tests/code/edgeidentity/build.gradle#L68

@cacheung
Copy link
Contributor

We don't need static final String BOOTED = "com.adobe.eventSource.booted"; in constant file.
https://github.com/prudrabhat/aepsdk-edgeidentity-android/blob/dev-v2.0.0_unit_functional_tests/code/edgeidentity/src/main/java/com/adobe/marketing/mobile/edge/identity/IdentityConstants.java#L29

@cacheung
Copy link
Contributor

The functional test on ci is failing?

@prudrabhat
Copy link
Contributor Author

The functional test on ci is failing?

Added the reason here : #78 (comment)

@prudrabhat
Copy link
Contributor Author

For this deepCopy https://github.com/prudrabhat/aepsdk-edgeidentity-android/blob/dev-v2.0.0_unit_functional_tests/code/edgeidentity/src/main/java/com/adobe/marketing/mobile/edge/identity/Utils.java#L61

Can we use Core's? https://github.com/adobe/aepsdk-core-android/blob/dev-v2.0.0/code/android-core-library/src/main/java/com/adobe/marketing/mobile/util/EventDataUtils.java#L137

Thanks for catching this. It turns out that the deep copy method is no longer used. It was being used by the toMap(JSONObject) and toList(JSONObject) methods and now since they are being used from Core no usages are left. I removed deepcopy() and its tests entirely now.

@codecov
Copy link

codecov bot commented Dec 15, 2022 •
edited
Loading

Codecov Report

Merging #78 (734d4b6) into dev-v2.0.0 (10c43ea) will increase coverage by 11.64%.
The diff coverage is 93.80%.

@@               Coverage Diff               @@
##           dev-v2.0.0      #78       +/-   ##
===============================================
+ Coverage       81.69%   93.33%   +11.64%     
===============================================
  Files              20       12        -8     
  Lines            1027      675      -352     
  Branches          151      103       -48     
===============================================
- Hits              839      630      -209     
+ Misses            132       16      -116     
+ Partials           56       29       -27
Flag Coverage Δ
functional-tests 67.41% <59.09%> (+6.55%) ⬆️
unit-tests 93.04% <92.98%> (+13.87%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...adobe/marketing/mobile/edge/identity/URLUtils.java 80.95% <50.00%> (ø)
...e/marketing/mobile/edge/identity/IdentityItem.java 67.44% <71.43%> (+6.57%) ⬆️
...eting/mobile/edge/identity/IdentityProperties.java 89.33% <71.43%> (-0.54%) ⬇️
...adobe/marketing/mobile/edge/identity/Identity.java 92.04% <91.67%> (+19.17%) ⬆️
...be/marketing/mobile/edge/identity/IdentityMap.java 97.56% <92.86%> (+10.99%) ⬆️
.../marketing/mobile/edge/identity/IdentityState.java 95.83% <94.44%> (+6.51%) ⬆️
...keting/mobile/edge/identity/IdentityExtension.java 99.15% <98.48%> (+21.31%) ⬆️
...eting/mobile/edge/identity/AuthenticatedState.java 100.00% <100.00%> (ø)
...com/adobe/marketing/mobile/edge/identity/ECID.java 95.24% <100.00%> (ø)
...obe/marketing/mobile/edge/identity/EventUtils.java 93.75% <100.00%> (+20.31%) ⬆️
... and 9 more

cacheung
cacheung approved these changes Dec 15, 2022
View reviewed changes
Copy link
Contributor

@cacheung cacheung left a comment

Choose a reason for hiding this comment

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

93% code coverage! Nice!

@prudrabhat prudrabhat merged commit afebb93 into adobe:dev-v2.0.0 Dec 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@addb addb addb approved these changes

@cacheung cacheung cacheung approved these changes

@kevinlind kevinlind Awaiting requested review from kevinlind

@emdobrin emdobrin Awaiting requested review from emdobrin

@timkimadobe timkimadobe Awaiting requested review from timkimadobe

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@prudrabhat @cacheung @addb