Skip to content

fix: update to latest build tools. slight refactor of datafile service. #335

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

Merged
merged 11 commits into from
Jun 18, 2020
Merged

fix: update to latest build tools. slight refactor of datafile service. #335

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
fe953f7
retool the datafile service.
thomaszurkan-optimizely Jun 12, 2020
7665539
fix tests
thomaszurkan-optimizely Jun 12, 2020
38dcfa4
update travis build tools
thomaszurkan-optimizely Jun 15, 2020
be4affd
change api as well as build tools
thomaszurkan-optimizely Jun 15, 2020
030b70c
cleanup gradle files
thomaszurkan-optimizely Jun 15, 2020
1672dd9
remove unused import
thomaszurkan-optimizely Jun 15, 2020
31268df
refactor to not allow more than one datafile download within a minute
thomaszurkan-optimizely Jun 16, 2020
0d98dbb
update to add another test for double download
thomaszurkan-optimizely Jun 18, 2020
a41cba4
added a test for when two downloads come in longer than minimum. cha…
thomaszurkan-optimizely Jun 18, 2020
0a53320
add a unit test for threading in datafile download
thomaszurkan-optimizely Jun 18, 2020
bfff4a0
remove unused imports
thomaszurkan-optimizely Jun 18, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions .travis.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,9 @@ dist: trusty
env:
global:
# These parameters should match the parameters for build tools and sdk versions in the gradle file
- ANDROID_BUILD_TOOLS=28.0.3 # should match gradle
- ANDROID_BUILD_TOOLS=29.0.3 # should match gradle
- ADB_INSTALL_TIMEOUT=5 # minutes
- ANDROID_API=28 # api is same as gradle file
- ANDROID_API=29 # api is same as gradle file
matrix:
- EMULATOR_API=21
- EMULATOR_API=22
Expand Down
98 changes: 80 additions & 18 deletions android-sdk/src/androidTest/java/com/optimizely/ab/android/sdk/OptimizelyManagerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -509,7 +509,7 @@ public void injectOptimizelyDoesNotDuplicateCallback() {
@Test
public void initializeSyncWithUpdateOnNewDatafileDisabled() {
boolean downloadToCache = true;
boolean updateConfigOnNewDatafiel = false;
boolean updateConfigOnNewDatafile = false;
int pollingInterval = 0; // disable polling

DefaultDatafileHandler datafileHandler = spy(new DefaultDatafileHandler());
Expand All @@ -528,7 +528,7 @@ public Object answer(InvocationOnMock invocation) {
}
}).when(manager.getDatafileHandler()).downloadDatafile(any(Context.class), any(DatafileConfig.class), any(DatafileLoadedListener.class));

OptimizelyClient client = manager.initialize(context, defaultDatafile, downloadToCache, updateConfigOnNewDatafiel);
OptimizelyClient client = manager.initialize(context, defaultDatafile, downloadToCache, updateConfigOnNewDatafile);

try {
executor.awaitTermination(1, TimeUnit.SECONDS);
Expand All @@ -542,7 +542,7 @@ public Object answer(InvocationOnMock invocation) {
@Test
public void initializeSyncWithUpdateOnNewDatafileEnabled() {
boolean downloadToCache = true;
boolean updateConfigOnNewDatafiel = true;
boolean updateConfigOnNewDatafile = true;
int pollingInterval = 0; // disable polling

DefaultDatafileHandler datafileHandler = spy(new DefaultDatafileHandler());
Expand All @@ -561,7 +561,7 @@ public Object answer(InvocationOnMock invocation) {
}
}).when(manager.getDatafileHandler()).downloadDatafile(any(Context.class), any(DatafileConfig.class), any(DatafileLoadedListener.class));

OptimizelyClient client = manager.initialize(context, defaultDatafile, downloadToCache, updateConfigOnNewDatafiel);
OptimizelyClient client = manager.initialize(context, defaultDatafile, downloadToCache, updateConfigOnNewDatafile);

try {
executor.awaitTermination(1, TimeUnit.SECONDS);
Expand All @@ -575,7 +575,7 @@ public Object answer(InvocationOnMock invocation) {
@Test
public void initializeSyncWithDownloadToCacheDisabled() {
boolean downloadToCache = false;
boolean updateConfigOnNewDatafiel = true;
boolean updateConfigOnNewDatafile = true;
int pollingInterval = 0; // disable polling

DefaultDatafileHandler datafileHandler = spy(new DefaultDatafileHandler());
Expand All @@ -594,7 +594,7 @@ public Object answer(InvocationOnMock invocation) {
}
}).when(manager.getDatafileHandler()).downloadDatafile(any(Context.class), any(DatafileConfig.class), any(DatafileLoadedListener.class));

OptimizelyClient client = manager.initialize(context, defaultDatafile, downloadToCache, updateConfigOnNewDatafiel);
OptimizelyClient client = manager.initialize(context, defaultDatafile, downloadToCache, updateConfigOnNewDatafile);

try {
executor.awaitTermination(1, TimeUnit.SECONDS);
Expand All @@ -608,7 +608,39 @@ public Object answer(InvocationOnMock invocation) {
@Test
public void initializeSyncWithUpdateOnNewDatafileDisabledWithPeriodicPollingEnabled() {
boolean downloadToCache = true;
boolean updateConfigOnNewDatafiel = false;
boolean updateConfigOnNewDatafile = false;
int pollingInterval = 30; // enable polling

DefaultDatafileHandler datafileHandler = spy(new DefaultDatafileHandler());
Logger logger = mock(Logger.class);
Context context = InstrumentationRegistry.getTargetContext();

OptimizelyManager manager = new OptimizelyManager(testProjectId, testSdkKey, null, logger, pollingInterval, datafileHandler, null, 0,
null, null, null, null);

doAnswer(
(Answer<Object>) invocation -> {
String newDatafile = manager.getDatafile(context, R.raw.datafile_api);
datafileHandler.saveDatafile(context, manager.getDatafileConfig(), newDatafile);
return null;
}).when(manager.getDatafileHandler()).downloadDatafile(any(Context.class), any(DatafileConfig.class), any(DatafileLoadedListener.class));

OptimizelyClient client = manager.initialize(context, defaultDatafile, downloadToCache, updateConfigOnNewDatafile);

try {
executor.awaitTermination(1, TimeUnit.SECONDS);
} catch (InterruptedException e) {
//
}

// when periodic polling enabled, project config always updated on cache datafile update (regardless of "updateConfigOnNewDatafile" setting)
assertEquals(client.getOptimizelyConfig().getRevision(), "241"); // wait for first download.
}

@Test
public void initializeSyncWithUpdateOnNewDatafileEnabledWithPeriodicPollingEnabled() {
boolean downloadToCache = true;
boolean updateConfigOnNewDatafile = true;
int pollingInterval = 30; // enable polling

DefaultDatafileHandler datafileHandler = spy(new DefaultDatafileHandler());
Expand All @@ -627,23 +659,22 @@ public Object answer(InvocationOnMock invocation) {
}
}).when(manager.getDatafileHandler()).downloadDatafile(any(Context.class), any(DatafileConfig.class), any(DatafileLoadedListener.class));

OptimizelyClient client = manager.initialize(context, defaultDatafile, downloadToCache, updateConfigOnNewDatafiel);
OptimizelyClient client = manager.initialize(context, defaultDatafile, downloadToCache, updateConfigOnNewDatafile);

try {
executor.awaitTermination(1, TimeUnit.SECONDS);
} catch (InterruptedException e) {
//
}

// when periodic polling enabled, project config always updated on cache datafile update (regardless of "updateConfigOnNewDatafile" setting)
assertEquals(client.getOptimizelyConfig().getRevision(), "241");
}

@Test
public void initializeSyncWithUpdateOnNewDatafileEnabledWithPeriodicPollingEnabled() {
public void initializeSyncWithUpdateOnNewDatafileDisabledWithPeriodicPollingDisabled() {
boolean downloadToCache = true;
boolean updateConfigOnNewDatafiel = true;
int pollingInterval = 30; // enable polling
boolean updateConfigOnNewDatafile = false;
int pollingInterval = 0; // disable polling

DefaultDatafileHandler datafileHandler = spy(new DefaultDatafileHandler());
Logger logger = mock(Logger.class);
Expand All @@ -661,7 +692,7 @@ public Object answer(InvocationOnMock invocation) {
}
}).when(manager.getDatafileHandler()).downloadDatafile(any(Context.class), any(DatafileConfig.class), any(DatafileLoadedListener.class));

OptimizelyClient client = manager.initialize(context, defaultDatafile, downloadToCache, updateConfigOnNewDatafiel);
OptimizelyClient client = manager.initialize(context, defaultDatafile, downloadToCache, updateConfigOnNewDatafile);

try {
executor.awaitTermination(1, TimeUnit.SECONDS);
Expand All @@ -670,13 +701,46 @@ public Object answer(InvocationOnMock invocation) {
}

// when periodic polling enabled, project config always updated on cache datafile update (regardless of "updateConfigOnNewDatafile" setting)
assertEquals(client.getOptimizelyConfig().getRevision(), "7"); // wait for first download.
}

@Test
public void initializeSyncWithUpdateOnNewDatafileEnabledWithPeriodicPollingDisabled() {
boolean downloadToCache = true;
boolean updateConfigOnNewDatafile = true;
int pollingInterval = 0; // disable polling

DefaultDatafileHandler datafileHandler = spy(new DefaultDatafileHandler());
Logger logger = mock(Logger.class);
Context context = InstrumentationRegistry.getTargetContext();

OptimizelyManager manager = new OptimizelyManager(testProjectId, testSdkKey, null, logger, pollingInterval, datafileHandler, null, 0,
null, null, null, null);

doAnswer(
new Answer<Object>() {
public Object answer(InvocationOnMock invocation) {
String newDatafile = manager.getDatafile(context, R.raw.datafile_api);
datafileHandler.saveDatafile(context, manager.getDatafileConfig(), newDatafile);
return null;
}
}).when(manager.getDatafileHandler()).downloadDatafile(any(Context.class), any(DatafileConfig.class), any(DatafileLoadedListener.class));

OptimizelyClient client = manager.initialize(context, defaultDatafile, downloadToCache, updateConfigOnNewDatafile);

try {
executor.awaitTermination(1, TimeUnit.SECONDS);
} catch (InterruptedException e) {
//
}

assertEquals(client.getOptimizelyConfig().getRevision(), "241");
}

@Test
public void initializeSyncWithResourceDatafileNoCache() {
boolean downloadToCache = true;
boolean updateConfigOnNewDatafiel = true;
boolean updateConfigOnNewDatafile = true;
int pollingInterval = 30; // enable polling

DefaultDatafileHandler datafileHandler = spy(new DefaultDatafileHandler());
Expand All @@ -687,15 +751,13 @@ public void initializeSyncWithResourceDatafileNoCache() {
null, null, null, null));

datafileHandler.removeSavedDatafile(context, manager.getDatafileConfig());
OptimizelyClient client = manager.initialize(context, R.raw.datafile, downloadToCache, updateConfigOnNewDatafiel);
OptimizelyClient client = manager.initialize(context, R.raw.datafile, downloadToCache, updateConfigOnNewDatafile);

verify(manager).initialize(eq(context), eq(defaultDatafile), eq(downloadToCache), eq(updateConfigOnNewDatafiel));
verify(manager).initialize(eq(context), eq(defaultDatafile), eq(downloadToCache), eq(updateConfigOnNewDatafile));
}

@Test
public void initializeSyncWithResourceDatafileNoCacheWithDefaultParams() {
boolean downloadToCache = true;
boolean updateConfigOnNewDatafiel = true;
int pollingInterval = 30; // enable polling

DefaultDatafileHandler datafileHandler = spy(new DefaultDatafileHandler());
Expand Down
6 changes: 5 additions & 1 deletion android-sdk/src/main/java/com/optimizely/ab/android/sdk/OptimizelyManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -491,8 +491,12 @@ public DatafileHandler getDatafileHandler() {
return datafileHandler;
}

private boolean datafileDownloadEnabled() {
return datafileDownloadInterval > 0;
}

private void startDatafileHandler(Context context) {
if (datafileDownloadInterval <= 0) {
if (!datafileDownloadEnabled()) {
logger.debug("Invalid download interval, ignoring background updates.");
return;
}
Expand Down
6 changes: 3 additions & 3 deletions build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,10 +49,10 @@ allprojects {
}

ext {
compile_sdk_version = 28
build_tools_version = "28.0.3"
compile_sdk_version = 29
build_tools_version = "29.0.3"
min_sdk_version = 14
target_sdk_version = 28
target_sdk_version = 29
java_core_ver = "3.4.3"
android_logger_ver = "1.3.6"
jacksonversion= "2.9.9.1"
Expand Down
122 changes: 120 additions & 2 deletions ...r/src/androidTest/java/com/optimizely/ab/android/datafile_handler/DatafileLoaderTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,8 @@
import org.slf4j.Logger;

import java.io.IOException;
import java.lang.reflect.Field;
import java.lang.reflect.Modifier;
import java.net.MalformedURLException;
import java.util.concurrent.TimeUnit;

Expand All @@ -45,8 +47,10 @@
import static junit.framework.Assert.fail;
import static org.mockito.Matchers.any;
import static org.mockito.Matchers.anyInt;
import static org.mockito.Mockito.atLeast;
import static org.mockito.Mockito.atMost;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.never;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.when;

Expand Down Expand Up @@ -150,7 +154,7 @@ public void noCacheAndLoadFromCDNFails() {
public void warningsAreLogged() throws IOException {
final ListeningExecutorService executor = MoreExecutors.newDirectExecutorService();
Cache cache = mock(Cache.class);
datafileCache = new DatafileCache("1", cache, logger);
datafileCache = new DatafileCache("warningsAreLogged", cache, logger);
DatafileLoader datafileLoader =
new DatafileLoader(datafileService, datafileClient, datafileCache, executor, logger);

Expand All @@ -159,7 +163,7 @@ public void warningsAreLogged() throws IOException {
when(cache.delete(datafileCache.getFileName())).thenReturn(false);
when(cache.save(datafileCache.getFileName(), "{}")).thenReturn(false);

datafileLoader.getDatafile("1", datafileLoadedListener);
datafileLoader.getDatafile("warningsAreLogged", datafileLoadedListener);
try {
executor.awaitTermination(5, TimeUnit.SECONDS);
} catch (InterruptedException e) {
Expand All @@ -170,4 +174,118 @@ public void warningsAreLogged() throws IOException {
verify(logger).warn("Unable to save new datafile");
verify(datafileLoadedListener, atMost(1)).onDatafileLoaded("{}");
}

@Test
public void debugLogged() throws IOException {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add one more test to check if back-to-back download works fine for both requests when they're apart 61secs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't want to add another an extra minute to the tests to prove this point.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep it's not great to add a minute wait time for testing. But this test must be important to make sure we do not freeze polling permanently by error. We can change it to inject the required gap time so can test with a reasonable wait time of a few seconds.
I'm ok to release it as is for now and then add the extra test with the next coming release.

final ListeningExecutorService executor = MoreExecutors.newDirectExecutorService();
Cache cache = mock(Cache.class);
datafileCache = new DatafileCache("debugLogged", cache, logger);
DatafileLoader datafileLoader =
new DatafileLoader(datafileService, datafileClient, datafileCache, executor, logger);

when(client.execute(any(Client.Request.class), anyInt(), anyInt())).thenReturn("{}");
when(cache.exists(datafileCache.getFileName())).thenReturn(true);
when(cache.delete(datafileCache.getFileName())).thenReturn(false);
when(cache.save(datafileCache.getFileName(), "{}")).thenReturn(false);

datafileLoader.getDatafile("debugLogged", datafileLoadedListener);
datafileLoader.getDatafile("debugLogged", datafileLoadedListener);
try {
executor.awaitTermination(5, TimeUnit.SECONDS);
} catch (InterruptedException e) {
fail();
}

verify(logger).debug("Last download happened under 1 minute ago. Throttled to be at least 1 minute apart.");
verify(datafileLoadedListener, atMost(2)).onDatafileLoaded("{}");
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's also check if it's called at least once

Suggested change
verify(datafileLoadedListener, atMost(2)).onDatafileLoaded("{}");
verify(datafileLoadedListener, atMost(2)).onDatafileLoaded("{}");
verify(datafileLoadedListener, atLeast(1)).onDatafileLoaded("{}");

verify(datafileLoadedListener, atLeast(1)).onDatafileLoaded("{}");
}

@Test
public void debugLoggedMultiThreaded() throws IOException {
final ListeningExecutorService executor = MoreExecutors.newDirectExecutorService();
Cache cache = mock(Cache.class);
datafileCache = new DatafileCache("debugLoggedMultiThreaded", cache, logger);
DatafileLoader datafileLoader =
new DatafileLoader(datafileService, datafileClient, datafileCache, executor, logger);

when(client.execute(any(Client.Request.class), anyInt(), anyInt())).thenReturn("{}");
when(cache.exists(datafileCache.getFileName())).thenReturn(true);
when(cache.delete(datafileCache.getFileName())).thenReturn(true);
when(cache.load(datafileCache.getFileName())).thenReturn("{}");
when(cache.save(datafileCache.getFileName(), "{}")).thenReturn(true);

Runnable r = () -> datafileLoader.getDatafile("debugLoggedMultiThreaded", datafileLoadedListener);

new Thread(r).start();
new Thread(r).start();
new Thread(r).start();
new Thread(r).start();

datafileLoader.getDatafile("debugLoggedMultiThreaded", datafileLoadedListener);

try {
executor.awaitTermination(5, TimeUnit.SECONDS);
} catch (InterruptedException e) {
fail();
}

verify(logger, atLeast(1)).debug("Last download happened under 1 minute ago. Throttled to be at least 1 minute apart.");
verify(datafileLoadedListener, atMost(4)).onDatafileLoaded("{}");
verify(datafileLoadedListener, atLeast(1)).onDatafileLoaded("{}");
}


private void setTestDownloadFrequency(DatafileLoader datafileLoader, long value) {
try {
Field betweenDownloadsMilli = DatafileLoader.class.getDeclaredField("minTimeBetweenDownloadsMilli");
betweenDownloadsMilli.setAccessible(true);

//Field modifiersField;
//modifiersField = Field.class.getDeclaredField("modifiers");
//modifiersField.setAccessible(true);
//modifiersField.setInt(betweenDownloadsMilli, betweenDownloadsMilli.getModifiers() & ~Modifier.FINAL);
betweenDownloadsMilli.set(null, value);

} catch (NoSuchFieldException e) {
e.printStackTrace();
}
catch (IllegalAccessException e) {
e.printStackTrace();
}

}

@Test
public void allowDoubleDownload() throws IOException {
final ListeningExecutorService executor = MoreExecutors.newDirectExecutorService();
Cache cache = mock(Cache.class);
datafileCache = new DatafileCache("allowDoubleDownload", cache, logger);
DatafileLoader datafileLoader =
new DatafileLoader(datafileService, datafileClient, datafileCache, executor, logger);

// set download time to 1 second
setTestDownloadFrequency(datafileLoader, 1000L);

when(client.execute(any(Client.Request.class), anyInt(), anyInt())).thenReturn("{}");
when(cache.exists(datafileCache.getFileName())).thenReturn(true);
when(cache.delete(datafileCache.getFileName())).thenReturn(false);
when(cache.save(datafileCache.getFileName(), "{}")).thenReturn(false);

datafileLoader.getDatafile("allowDoubleDownload", datafileLoadedListener);
try {
executor.awaitTermination(2, TimeUnit.SECONDS);
} catch (InterruptedException e) {
fail();
}

datafileLoader.getDatafile("allowDoubleDownload", datafileLoadedListener);

// reset back to normal.
setTestDownloadFrequency(datafileLoader, 60 * 1000L);

verify(logger, never()).debug("Last download happened under 1 minute ago. Throttled to be at least 1 minute apart.");
verify(datafileLoadedListener, atMost(2)).onDatafileLoaded("{}");
verify(datafileLoadedListener, atLeast(1)).onDatafileLoaded("{}");
}
}
Loading