Fix #2780: Call checkIfLoadingIsPossible() after inject() in LessonThumbnailImageView

[ArpitShukIa]

Explanation

Fixes #2780: The problem was that few dependencies weren't available at the time of calling checkIfLoadingIsPossible() so loadLessonThumbnail() was not getting called for that particular image. I simply added a call to checkIfLoadingIsPossible() after Dagger inject() call and that fixed the issue.

Test Results:

Before:

Robolectric:

Expresso:

After:

Robolectric:

Expresso:

UI Results:

Before:

Before.mp4

After:

After.mp4

Checklist

• The PR title starts with "Fix #bugnum: ", followed by a short, clear summary of the changes. (If this PR fixes part of an issue, prefix the title with "Fix part of #bugnum: ...".)
• The PR explanation includes the words "Fixes #bugnum: ..." (or "Fixes part of #bugnum" if the PR only partially fixes an issue).
• The PR follows the style guide.
• The PR does not contain any unnecessary auto-generated code from Android Studio.
• The PR is made from a branch that's not called "develop".
• The PR is made from a branch that is up-to-date with "develop".
• The PR's branch is based on "develop" and not on any other branch.
• The PR is assigned to an appropriate reviewer in both the Assignees and the Reviewers sections.

[ArpitShukIa]

@anandwana001 PTAL.

[ArpitShukIa]

The lint test has failed because of the semi-colon that I used in the empty while loop. That was necessary to mark that loop as empty.
Should I use { } instead?

[anandwana001]

I guess {}; will prevent the failure. Also, please complete all the installation process.

[ArpitShukIa]

@anandwana001 Sorry, I don't understand what you mean by installation process.

[FareesHussain]

@anandwana001 Sorry, I don't understand what you mean by installation process.

To setup pre-push hooks to prevent the ktlint check errors

Ref: 4th point in https://github.com/oppia/oppia-android/wiki#installation

[anandwana001]

Deferring to Rajat for inital pass, as he is the codeowner.

[rt4914]

@ArpitShukIa Adding an infinite loop doesn't seem like a correct approach to me at the same time I understand this issue is not that easy too.

@BenHenning Do you have any comments on this approach?

[BenHenning]

Hi @ArpitShukIa. Left one comment--PTAL when you can.

[BenHenning]

@ArpitShukIa I believe you have a few errors to correct in CI:

/home/runner/work/oppia-android/oppia-android/app/build/tmp/kapt3/stubs/debugUnitTest/org/oppia/android/app/customview/LessonThumbnailImageViewTest.java:73: error: [Dagger/MissingBinding] @org.oppia.android.app.topic.EnablePracticeTab java.lang.Boolean cannot be provided without an @Provides-annotated method.

I suggest making sure the new test builds & passes locally before sending this back to reviewers. This is probably just an issue from syncing to the latest develop changes (I suggest looking at other tests to see how they fill in these bindings).

Also, can you confirm whether the new test is passing on Espresso? That + the green CI are my main blockers before approving the PR since it otherwise LGTM.

[anandwana001]

LGTM, as per my earlier comment.

[ArpitShukIa]

Also, can you confirm whether the new test is passing on Espresso? That + the green CI are my main blockers before approving the PR since it otherwise LGTM.

All the checks have passed but the Expresso problem is still there. The tests keep on running indefinitely.

Expresso.tests.mov

[BenHenning]

@ArpitShukIa we might need to make this a Robolectric-only test. I'm not entirely sure why it doesn't work on Espresso, but given that the original regression triggers a test failure in Robolectric I think it being Robolectric-only is fine. Can you move it to be in the same directory structure except test/... instead of sharedTest/...?

[BenHenning]

Though thinking on this more, it either indicates a real issue that Robolectric isn't covering, or it's not liking how we're creating the view.

@ArpitShukIa you should also step-debug the Espresso test if you can to try and narrow down where the test is starting to hang. I think those findings should be identified before we move the test (though that's a valid option if we can't solve the Espresso issue).

[ArpitShukIa]

@BenHenning The expresso tests are passing now :)
I just had to make the assertions outside the activityTestRule.runOnUiThread {...} block.

[BenHenning]

Ah, that's really subtle @ArpitShukIa. Nice find. Could you actually file an issue to the thread of: we should introduce a static analysis check or somehow trigger test failures in these situations to avoid the test hanging? It might require us to wrap Espresso's API, but I think that'd be worth it to avoid people running into this in the future. This seems tricky to debug/track down.

[BenHenning]

Given that the tests are now passing for both Robolectric & Espresso, this LGTM. Thanks @ArpitShukIa!

[BenHenning]

@rt4914 sending this over to you since I think only your review is needed now.

[ArpitShukIa]

Could you actually file an issue to the thread of: we should introduce a static analysis check or somehow trigger test failures in these situations to avoid the test hanging?

@BenHenning I am a little confused about the description of the issue. Could you instead file the issue plz?

The problem was that activityTestRule.runOnUiThread {...} block runs on UIThread and onView waits for the UIThread to idle. Calling onView() inside that block results in a kind of deadlock and the test execution gets stuck.

[rt4914]

LGTM, thanks.

[BenHenning]

@ArpitShukIa we should have a test or check to ensure that such cases fail immediately with a good error message for developers to fix the situation rather than running into the deadlock (the deadlock is exactly what we want to avoid since it's difficult to understand what's going wrong). Static analysis or specific failure tests are a good way to prevent these sorts of situations from happening.

Edit: I went ahead and filed #3150 to make sure this context is captured.