Update Functional Tests, use Core TimeUtils date formatter, remove Jetifier #50

cacheung · 2022-12-19T16:58:09Z

Update Functional Tests:

Change the folder structure, move all Util files under to util folder
Update all the test constants to ConsentTestConstants
Update Util files for 2.0 changes

Updated to use Core TimeUtils date format.

Update the remaining powermock code and dependencies.

Remove Jetifier.

Description

Related Issue

Motivation and Context

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.

Update Functional test

…white box code Use Core TimeUtils for data formatter and remove remaining powermock white box code

Update Functional Tests

…oid into functionalTest

Test clean up

remove a commented code

cacheung · 2022-12-19T17:00:04Z

code/edgeconsent/src/main/java/com/adobe/marketing/mobile/edge/consent/Consents.java

-		metaDataContents.put(ConsentConstants.EventDataKey.TIME, TimeUtils.getIso8601DateTimeZoneISO8601());
+		metaDataContents.put(
+			ConsentConstants.EventDataKey.TIME,
+			TimeUtils.getISO8601UTCDateWithMilliseconds(new Date(timeStamp))


Use Core TimeUtils here.

This should be addressed with the latest update

code/edgeconsent/src/test/java/com/adobe/marketing/mobile/edge/consent/ConsentsTest.java

…tions() Fix functional tests, add wait time and remove extra resetTestExpectations()

codecov · 2022-12-19T21:14:20Z

Codecov Report

Merging #50 (f3a3dfd) into dev-v2.0.0 (8a6fa05) will increase coverage by 8.89%.
The diff coverage is 82.86%.

@@              Coverage Diff               @@
##           dev-v2.0.0      #50      +/-   ##
==============================================
+ Coverage       75.88%   84.77%   +8.89%     
==============================================
  Files              11        4       -7     
  Lines             398      256     -142     
  Branches           44       30      -14     
==============================================
- Hits              302      217      -85     
+ Misses             67       23      -44     
+ Partials           29       16      -13

Flag	Coverage Δ
functional-tests	`66.80% <67.14%> (+10.26%)`	⬆️
unit-tests	`82.81% <77.14%> (+9.45%)`	⬆️

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

Impacted Files	Coverage Δ
.../adobe/marketing/mobile/edge/consent/Consents.java	`76.47% <57.14%> (-2.80%)`	⬇️
...m/adobe/marketing/mobile/edge/consent/Consent.java	`78.05% <77.78%> (+16.35%)`	⬆️
...arketing/mobile/edge/consent/ConsentExtension.java	`88.89% <93.94%> (+17.34%)`	⬆️
.../marketing/mobile/edge/consent/ConsentManager.java	`100.00% <100.00%> (ø)`

timkimadobe

Looks good to me!

Use Jitpack Assurance and Remove Jetifier

prudrabhat · 2022-12-20T02:19:28Z

code/edgeconsent/src/main/java/com/adobe/marketing/mobile/edge/consent/ConsentConstants.java

@@ -31,6 +29,8 @@ static final class EventDataKey {
 		private EventDataKey() {}
 	}

+	private ConsentConstants() {}


nit: can you move constructor to line 20.

Moved constructor for ConsentConstants above static final class definitions!

prudrabhat · 2022-12-20T19:45:53Z

code/edgeconsent/src/main/java/com/adobe/marketing/mobile/edge/consent/ConsentExtension.java

-	private final ConsentManager consentManager;
+
+	// package private vars
+	ConsentManager consentManager;


I suggest injecting this variable via constructor and retain its final nature while making it testable. That way you can mock this directly in your test as well as eliminate the whitebox mock.

protected ConsentExtension(final ExtensionApi extensionApi) { this( extensionApi, ServiceProvider .getInstance() .getDataStoreService() .getNamedCollection(ConsentConstants.DataStoreKey.DATASTORE_NAME) ); } @VisibleForTesting ConsentExtension(final ExtensionApi extensionApi, final NamedCollection namedCollection) { this(extensionApi, new ConsentManager(namedCollection)) } protected ConsentExtension(final ExtensionApi extensionApi, final ConsentManager consentManager) { super(extensionApi); this.consentManager = consentManager; }

Thank you Prashanth! Refactored this with the suggested implementation and updated the related test cases

… injection Update test helper method that sets up persistence using new constructor Update ConsentConstants constructor position

timkimadobe

Thanks for the review @prudrabhat! I noticed during the implementation of the refactor for ConsentExtension constructor, when giving protected status even without the @VisibleForTesting annotation, it is still accessible to the test cases. So I defined the constructor like this:

protected ConsentExtension(final ExtensionApi extensionApi, final NamedCollection namedCollection)

Also, I wasn't able to fully remove the setupExistingConsents helper function in ConsentExtensionTest because it handles the alternate test case setup flow where the mocked NamedCollection needs to have its getString API mocked with the provided jsonString to simulate the situation where there are consents already in persistence.

I noticed in the existing ConsentExtension logic that this fetch from persistence by the ConsentManager is only done once on initialization, so having this helper method to set the API mock with the required consents value and reinstantiate the ConsentExtension to trigger the read from persistence using the mocked NamedCollection was helpful to keep all the test cases which require this logical flow working. What do you think?

I also noticed that there is a similar case in ConsentManager where the property Consents defaultConsents is being accessed directly by test cases in order to perform the required assertions; is this also something we want to refactor in a similar way to the mutable package private consentManager?

timkimadobe · 2022-12-21T23:32:34Z

code/edgeconsent/src/main/java/com/adobe/marketing/mobile/edge/consent/ConsentConstants.java

@@ -31,6 +29,8 @@ static final class EventDataKey {
 		private EventDataKey() {}
 	}

+	private ConsentConstants() {}


Moved constructor for ConsentConstants above static final class definitions!

timkimadobe · 2022-12-22T01:22:29Z

code/edgeconsent/src/main/java/com/adobe/marketing/mobile/edge/consent/ConsentExtension.java

-	private final ConsentManager consentManager;
+
+	// package private vars
+	ConsentManager consentManager;


Thank you Prashanth! Refactored this with the suggested implementation and updated the related test cases

kevinlind · 2022-12-22T03:53:18Z

...androidTest/java/com/adobe/marketing/mobile/edge/consent/util/ConsentFunctionalTestUtil.java

+	 * @param configuration the initial configuration update that needs to be applied
+	 * @throws InterruptedException if the wait time for extension registration has elapsed
+	 */
+	public static void registerExtensions(


nit: Move to TestHelper.

Moved function and registration timeout constant to TestHelper, and refactored all usages!

kevinlind · 2022-12-22T04:01:20Z

...sent/src/androidTest/java/com/adobe/marketing/mobile/edge/consent/util/MonitorExtension.java

-	 * @param source the source of the event.
-	 * @param count the number of events expected to be received.
-	 */
-	public static void setExpectedEvent(final String type, final String source, final int count) {


Looks like the ability to set an "expected" event was removed from the MonitorExtension. If this functionality isn't needed, then can you remove the expectedEvents variable and any handling of checking for or returning expected events?

Removed expectedEvents property and all associated logic!

kevinlind · 2022-12-22T04:10:16Z

...src/androidTest/java/com/adobe/marketing/mobile/edge/consent/util/TestPersistenceHelper.java


 import static org.junit.Assert.fail;

 import android.app.Application;
 import android.content.Context;
 import android.content.SharedPreferences;
-import com.adobe.marketing.mobile.edge.consent.ConsentTestConstants;
 import java.util.ArrayList;

 /**


Is the TestPersistenceHelper class needed anymore? Can the test cases use a mocked DataStoreService instead of reading the persisted values from shared preferences?

If separating test cases from the actual SharedPreferences is something we want to pursue, is that something we could cover in a separate task? I feel that the scope is a bit large seeing that many different test cases interact with SharedPreferences by updating/reading values and we want to make sure to properly handle all those cases and also test the integration of Consent with the actual SharedPreferences

Yes, totally fine to take this up in a separate PR.

kevinlind · 2022-12-22T04:11:55Z

code/edgeconsent/src/main/java/com/adobe/marketing/mobile/edge/consent/ConsentExtension.java

@@ -49,14 +50,27 @@ protected ConsentExtension(final ExtensionApi extensionApi) {
 	}

 	/**
-	 * Constructor used for testing.
+	 * Convenience constructor that instantiates a new {@link ConsentManager} using the passed {@link NamedCollection}.
+	 * <br><br>


Use a single <p> tag to add a line break in JavaDoc.

Thank you! Updated to use the <p> tag

kevinlind · 2022-12-22T04:15:21Z

code/edgeconsent/src/main/java/com/adobe/marketing/mobile/edge/consent/ConsentManager.java

-	private Consents defaultConsents; // holds on to default consents obtained from configuration response
+
+	// package private vars
+	Consents defaultConsents; // holds on to default consents obtained from configuration response


Is this package-private for testing or production? Can @VisbleForTesting be used here?

I believe it's package-private for testing purposes! I've added the @VisibleForTesting annotation as suggested

kevinlind · 2022-12-22T04:18:28Z

.../edgeconsent/src/test/java/com/adobe/marketing/mobile/edge/consent/ConsentExtensionTest.java

+	/**
+	 * Sets up existing consents values in persistence by setting up a mocked return value for the
+	 * mocked {@link NamedCollection} using the passed JSON consents {@link String}.
+	 * <br><br>


Use <p> to add line break in JavaDocs.

Add VisibleForTesting annotation

timkimadobe

Thanks for the review @kevinlind! I've addressed the feedback with the latest updates, with one outstanding question on the refactor of TestPersistenceHelper

timkimadobe · 2022-12-22T19:29:09Z

...androidTest/java/com/adobe/marketing/mobile/edge/consent/util/ConsentFunctionalTestUtil.java

+	 * @param configuration the initial configuration update that needs to be applied
+	 * @throws InterruptedException if the wait time for extension registration has elapsed
+	 */
+	public static void registerExtensions(


Moved function and registration timeout constant to TestHelper, and refactored all usages!

timkimadobe · 2022-12-22T20:10:02Z

...sent/src/androidTest/java/com/adobe/marketing/mobile/edge/consent/util/MonitorExtension.java

-	 * @param source the source of the event.
-	 * @param count the number of events expected to be received.
-	 */
-	public static void setExpectedEvent(final String type, final String source, final int count) {


Removed expectedEvents property and all associated logic!

timkimadobe · 2022-12-22T20:32:07Z

code/edgeconsent/src/main/java/com/adobe/marketing/mobile/edge/consent/ConsentManager.java

-	private Consents defaultConsents; // holds on to default consents obtained from configuration response
+
+	// package private vars
+	Consents defaultConsents; // holds on to default consents obtained from configuration response


I believe it's package-private for testing purposes! I've added the @VisibleForTesting annotation as suggested

timkimadobe · 2022-12-22T20:34:07Z

code/edgeconsent/src/main/java/com/adobe/marketing/mobile/edge/consent/ConsentExtension.java

@@ -49,14 +50,27 @@ protected ConsentExtension(final ExtensionApi extensionApi) {
 	}

 	/**
-	 * Constructor used for testing.
+	 * Convenience constructor that instantiates a new {@link ConsentManager} using the passed {@link NamedCollection}.
+	 * <br><br>


Thank you! Updated to use the <p> tag

timkimadobe · 2022-12-22T20:43:17Z

...src/androidTest/java/com/adobe/marketing/mobile/edge/consent/util/TestPersistenceHelper.java


 import static org.junit.Assert.fail;

 import android.app.Application;
 import android.content.Context;
 import android.content.SharedPreferences;
-import com.adobe.marketing.mobile.edge.consent.ConsentTestConstants;
 import java.util.ArrayList;

 /**


If separating test cases from the actual SharedPreferences is something we want to pursue, is that something we could cover in a separate task? I feel that the scope is a bit large seeing that many different test cases interact with SharedPreferences by updating/reading values and we want to make sure to properly handle all those cases and also test the integration of Consent with the actual SharedPreferences

timkimadobe · 2022-12-22T20:45:19Z

code/edgeconsent/src/main/java/com/adobe/marketing/mobile/edge/consent/Consents.java

-		metaDataContents.put(ConsentConstants.EventDataKey.TIME, TimeUtils.getIso8601DateTimeZoneISO8601());
+		metaDataContents.put(
+			ConsentConstants.EventDataKey.TIME,
+			TimeUtils.getISO8601UTCDateWithMilliseconds(new Date(timeStamp))


This should be addressed with the latest update

cacheung added 6 commits December 16, 2022 00:17

Update Functional test

0dca4ee

Update Functional test

Use Core TimeUtils for data formatter and remove remaining powermock …

ad103d1

…white box code Use Core TimeUtils for data formatter and remove remaining powermock white box code

Update Functional Tests

d028608

Update Functional Tests

Merge branch 'dev-v2.0.0' of github.com:adobe/aepsdk-edgeconsent-andr…

3570f00

…oid into functionalTest

Test clean up

6aacbdf

Test clean up

remove a commented code

b3fccb6

remove a commented code

cacheung commented Dec 19, 2022

View reviewed changes

Fix functional tests, add wait time and remove extra resetTestExpecta…

1e5081d

…tions() Fix functional tests, add wait time and remove extra resetTestExpectations()

cacheung requested review from kevinlind, emdobrin, addb, timkimadobe and prudrabhat December 19, 2022 21:17

timkimadobe approved these changes Dec 19, 2022

View reviewed changes

Use Jitpack Assurance and Remove Jetifier

844a3e4

Use Jitpack Assurance and Remove Jetifier

cacheung changed the title ~~Update Functional Tests and use Core TimeUtils date formatter.~~ Update Functional Tests, use Core TimeUtils date formatter, remove Jetifier Dec 20, 2022

prudrabhat reviewed Dec 20, 2022

View reviewed changes

timkimadobe added 2 commits December 21, 2022 17:08

Update ConsentExtension with new constructor to allow for better mock…

210afbb

… injection Update test helper method that sets up persistence using new constructor Update ConsentConstants constructor position

Apply lint formatting

32132fc

timkimadobe reviewed Dec 22, 2022

View reviewed changes

kevinlind reviewed Dec 22, 2022

View reviewed changes

timkimadobe added 3 commits December 22, 2022 12:08

Move registerExtensions API to TestHelper

17276f6

Remove unused expectedEvents and related logic from MonitorExtension

f4b5ce3

Update line break style in javadocs to p

d554ca7

Add VisibleForTesting annotation

timkimadobe reviewed Dec 22, 2022

View reviewed changes

kevinlind approved these changes Dec 22, 2022

View reviewed changes

timkimadobe added 3 commits December 22, 2022 12:54

Apply lint formatting

c0cb8cf

Update dependencies to maven snapshot

78a73b8

Update dependencies

f3a3dfd

timkimadobe merged commit ac20c32 into adobe:dev-v2.0.0 Dec 22, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update Functional Tests, use Core TimeUtils date formatter, remove Jetifier #50

Update Functional Tests, use Core TimeUtils date formatter, remove Jetifier #50

cacheung commented Dec 19, 2022 •

edited

Loading

cacheung Dec 19, 2022

timkimadobe Dec 22, 2022

codecov bot commented Dec 19, 2022 •

edited

Loading

timkimadobe left a comment

prudrabhat Dec 20, 2022

timkimadobe Dec 21, 2022

prudrabhat Dec 20, 2022 •

edited

Loading

timkimadobe Dec 22, 2022

timkimadobe left a comment

timkimadobe Dec 21, 2022

timkimadobe Dec 22, 2022

kevinlind Dec 22, 2022

timkimadobe Dec 22, 2022

kevinlind Dec 22, 2022

timkimadobe Dec 22, 2022

kevinlind Dec 22, 2022

timkimadobe Dec 22, 2022

kevinlind Dec 22, 2022

kevinlind Dec 22, 2022

timkimadobe Dec 22, 2022

kevinlind Dec 22, 2022

timkimadobe Dec 22, 2022

kevinlind Dec 22, 2022

timkimadobe left a comment

timkimadobe Dec 22, 2022

timkimadobe Dec 22, 2022

timkimadobe Dec 22, 2022

timkimadobe Dec 22, 2022

timkimadobe Dec 22, 2022

timkimadobe Dec 22, 2022

Update Functional Tests, use Core TimeUtils date formatter, remove Jetifier #50

Update Functional Tests, use Core TimeUtils date formatter, remove Jetifier #50

Conversation

cacheung commented Dec 19, 2022 • edited Loading

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Checklist:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Dec 19, 2022 • edited Loading

Codecov Report

timkimadobe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prudrabhat Dec 20, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timkimadobe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timkimadobe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cacheung commented Dec 19, 2022 •

edited

Loading

codecov bot commented Dec 19, 2022 •

edited

Loading

prudrabhat Dec 20, 2022 •

edited

Loading