-
Notifications
You must be signed in to change notification settings - Fork 529
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.
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
Fix #2642: Fix all Data module Tests and add them to run on Github Actions CI check #2916
Fix #2642: Fix all Data module Tests and add them to run on Github Actions CI check #2916
Conversation
…stentCacheStoreTest.kt.
@BenHenning @jcqli PTAL. All test cases of the data module are now fixed except for the @BenHenning I have reformatted |
…MockFeedbackReportingTest.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All other fields look good, thanks for fixing this!
… fix-data-module-tests-and-add-them-to-ci
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, LGTM :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
...src/main/java/org/oppia/android/data/backends/gae/model/GaeFeedbackReportingSystemContext.kt
Outdated
Show resolved
Hide resolved
data/src/test/java/org/oppia/android/data/backends/test/MockClassroomTest.kt
Outdated
Show resolved
Hide resolved
Ah, just saw the comments above. |
@BenHenning They were failing only 4-5 time out of 100 times and the cause of failure was - retrofit2.mock.MockRetrofitIOException: Failure triggered by MockRetrofit's NetworkBehavior And, here is some part of the declaration of the /**
* A simple emulation of the behavior of network calls.
* <p>
* This class models three properties of a network:
* <ul>
* <li>Delay – the time it takes before a response is received (successful or otherwise).</li>
* <li>Variance – the amount of fluctuation of the delay to be faster or slower.</li>
* <li>Failure - the percentage of operations which fail (such as {@link IOException}).</li>
* </ul>
* Behavior can be applied to a Retrofit interface with {@link MockRetrofit}. Behavior can also
* be applied elsewhere using {@link #calculateDelay(TimeUnit)} and {@link #calculateIsFailure()}.
* <p>
* By default, instances of this class will use a 2 second delay with 40% variance. Failures
* will occur 3% of the time. HTTP errors will occur 0% of the time.
*/
public final class NetworkBehavior {
private static final int DEFAULT_DELAY_MS = 2000; // Network calls will take 2 seconds.
private static final int DEFAULT_VARIANCE_PERCENT = 40; // Network delay varies by ±40%.
private static final int DEFAULT_FAILURE_PERCENT = 3; // 3% of network calls will fail.
private static final int DEFAULT_ERROR_PERCENT = 0; // 0% of network calls will return errors.
....// Some more code By default, Also here is some more reference to the documentation of /** Set the percentage of calls to {@link #calculateIsFailure()} that return {@code true}. */
public void setFailurePercent(int failurePercent) {
checkPercentageValidity(failurePercent, "Failure percentage must be between 0 and 100.");
this.failurePercent = failurePercent;
}
/**
* Randomly determine whether this call should result in a network failure in accordance with
* configured behavior. When true, {@link #failureException()} should be thrown.
*/
public boolean calculateIsFailure() {
return random.nextInt(100) < failurePercent;
}
private static void checkPercentageValidity(int percentage, String message) {
if (percentage < 0 || percentage > 100) {
throw new IllegalArgumentException(message);
}
} So, first, we check if the failure percent is between 0 and 100 (default is 3). And if it between 3 and 100 we use it in the method /** @BenHenning Please go below near onRun() method, as we are not interested in this. */
final class BehaviorCall<T> implements Call<T> {
final NetworkBehavior behavior;
final ExecutorService backgroundExecutor;
final Call<T> delegate;
private volatile Future<?> task;
volatile boolean canceled;
@GuardedBy("this")
private boolean executed;
BehaviorCall(NetworkBehavior behavior, ExecutorService backgroundExecutor, Call<T> delegate) {
this.behavior = behavior;
this.backgroundExecutor = backgroundExecutor;
this.delegate = delegate;
}
@SuppressWarnings("CloneDoesntCallSuperClone") // We are a final type & this saves clearing state.
@Override public Call<T> clone() {
return new BehaviorCall<>(behavior, backgroundExecutor, delegate.clone());
}
@Override public Request request() {
return delegate.request();
}
@SuppressWarnings("ConstantConditions") // Guarding public API nullability.
@Override public void enqueue(final Callback<T> callback) {
if (callback == null) throw new NullPointerException("callback == null");
synchronized (this) {
if (executed) throw new IllegalStateException("Already executed");
executed = true;
}
task = backgroundExecutor.submit(new Runnable() {
boolean delaySleep() {
long sleepMs = behavior.calculateDelay(MILLISECONDS);
if (sleepMs > 0) {
try {
Thread.sleep(sleepMs);
} catch (InterruptedException e) {
callback.onFailure(BehaviorCall.this, new IOException("canceled"));
return false;
}
}
return true;
}
/** @BenHenning We are interested here. */
@Override public void run() {
if (canceled) {
callback.onFailure(BehaviorCall.this, new IOException("canceled"));
} else if (behavior.calculateIsFailure()) {
if (delaySleep()) {
callback.onFailure(BehaviorCall.this, behavior.failureException());
}
} else if (behavior.calculateIsError()) {
if (delaySleep()) {
//noinspection unchecked An error response has no body.
callback.onResponse(BehaviorCall.this, (Response<T>) behavior.createErrorResponse());
}
} else {
delegate.enqueue(new Callback<T>() {
@Override public void onResponse(Call<T> call, Response<T> response) {
if (delaySleep()) {
callback.onResponse(call, response);
}
}
@Override public void onFailure(Call<T> call, Throwable t) {
if (delaySleep()) {
callback.onFailure(call, t);
}
}
});
}
}
});
} So, if the |
…so refactor the corresponding filed of the data class GaeFeedbackReportingSystemContext.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, that's an excellent find @prayutsu! Might it be possible for us to introduce a new test module (e.g. in testing/ package) that creates & provides the mock retrofit object and can be shared among all of these tests? It'd be nice to set this configuration in one place so that we don't actually hit it in the future when we add more services.
@jcqli I think is working on something similar in a related PR, so it might make sense to collaborate to determine which PR should have that module in it.
For clarification, we would also want to move the mock services to the new module, right? So then both the retrofit mock and the services can be shared with each other |
…N value of platform_version.
… fix-data-module-tests-and-add-them-to-ci
data/src/test/java/org/oppia/android/data/backends/test/MockSubtopicTest.kt
Outdated
Show resolved
Hide resolved
testing/src/main/java/org/oppia/android/testing/network/MockRetrofitHelper.kt
Outdated
Show resolved
Hide resolved
@anandwana001 @BenHenning Data module's tests are failing locally for me on Bazel but they are passing in CI. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@prayutsu I can't check right now if they are working locally for me, but I suspect they will if they're passing on CI.
The failures are either the known compute affected tests flake tracked in #2691 & the other failure is #2032. No failures seem tied to this PR.
Otherwise, the PR LGTM. Just have 1 nit + 1 existing comment to address before approving.
kt_android_library( | ||
name = "retrofit_module", | ||
srcs = [ | ||
"RetrofitModule.kt", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah. Do you have JDK 8 installed locally? Bazel uses an embedded JDK 11 for actions & building, but for running it uses whatever version of JDK you have locally (we generally recommend Java 8, though CI uses 9 I believe). This seems to be a class version incompatibility, so installing JDK 8 & setting it as your default may fix this issue.
If you don't want to set it as your default, you can set JAVA_HOME temporarily before using bazel test
and it should use that version. There might be a way to configure this via bazelrc, as well, but I'm not sure.
… fix-data-module-tests-and-add-them-to-ci
@anandwana001 PTAL, all the checks are passing now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @prayutsu! The PR underwent a bunch of iterations, but I think it landed in a really nice place. LGTM!
Also as an aside: please don't mark any conversations in PRs resolved unless you authored them (it makes it harder for the person creating the conversation to make sure the changes were addressed).
@BenHenning Sure, I think I only resolved the conversations initiated by me, maybe I would have accidentally resolved some conversation authored by you, will take care of in the future. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
all test files LGTM
@BenHenning PR is ready to merge. |
Ah @prayutsu I think this was my mistake. You're right--the comment of mine I saw disappear was because it was a conversation thread you started. It looks like you did follow the process entirely--thanks! :) Really happy to see this getting merged. Merging it now. |
Explanation
Fixes #2642:
All the flaky data module tests are fixed and added to run on CI.
Checklist
Screenshots of all passing tests (Each test file is run 100 times)
QuestionPlayerServiceTest
FeedbackReportingServiceTest
SubtopicServiceTest
TopicServiceTest
StoryServiceTest
ExplorationServiceTest
ConecptCardServiceTest
ClassroomServiceTest
NetworkInterceptorTest
PersisitentCacheStoreTest
JsonNetwrokInterceptortest
RemoteAuthNetwrokInterceptortest
FeedbackReportingServiceTest was always failing.
Fixed FeedbackReportingServiceTest -