[RunAllTests] Fix part of #2787: Add support for proto lesson loading from local assets #2978

BenHenning · 2021-03-23T05:45:05Z

Fix part of #2787

This adds initial support for loading the new lessons needed for alpha. In particular, to better prepare the codebase for post-alpha beta work, this PR is introducing support for directly loading proto assets rather than JSON (bypassing the entire JSON loading pipeline). This requires punching a few holes into various retrievers/controllers (gated by a new qualified boolean: LoadLessonProtosFromAssets) to load proto rather than JSON. The resulting proto code is a bit simpler than the JSON versions since typing becomes much simpler.

In order to ensure the new pathways are (mostly) working in tests, this PR enables proto caching in some tests. However doing this isn't straightforward: we don't want to check actual binary proto files into the codebase since they are binary files and will unnecessarily bloat our repository. Textproto is a more reasonable alternative since it's plaintext (and actually a bit more compact than JSON), but complicated to use: proto lite does not support loading via TextFormat. Rather than introducing a dependency on JsonFormat (to avoid JSON altogether), I decided instead to introduce a custom Starlark rule in Bazel to, at build time, convert textproto files to binary files & include those at build time. This works quite well: for both the apk itself & tests we can load binary pb files as though we're including them directly. Also, for this reason is why the PR includes textproto conversions of our development assets.

However, the above approach only works for Bazel. To mitigate this, the few test suites that were updated to verify the fixes conditionally enable proto loading based on whether the environment is Bazel. This is a bit hacky, but it seems like the best solution in the interim until we can entirely move off of Gradle per #59.

Other considerations for this PR:

This removes dependence on test story 1 entirely because it includes structural elements that are incompatible with the pipeline used to convert to text/binary proto. This seems fine since we don't actually need these interactions for testing or development.
This removes support for confirmed unclassified answers since they aren't used at all, and probably won't be in Android.
This PR does not introduce support for the up-to-date interactions that will be shipped with the alpha. That will happen in a later PR (mainly [RunAllTests] Fix #2944, #2787, #298, #3007: Update interactions to support new localization structures #2985).
The textproto files introduced in this PR were generated using an in-progress script that's also being used to download the assets that will be used for alpha. Follow up with me if you want to see the script.
Enabling proto caching & running the APK locally will yield to images not loading. This is partly due to [RunAllTests] Fix #2941: Fix glide image loading in Bazel #2979 (in that they can't load without that fix), but mostly due to the fact that all of our dev assets short-circuit thumbnails (which is why we never discovered Locked lesson thumbnails don't blur when loaded as SVGs #2940 until working on alpha)

Note that I can't actually verify Espresso tests are working with these versions since the Bazel project doesn't yet have Espresso tests set up.

…oto-lesson-loading-from-local-assets

This will be added in a later branch in the chain.

BenHenning · 2021-03-24T09:53:27Z

/cc @vinitamurthi

vinitamurthi · 2021-03-24T14:51:26Z

domain/src/main/java/org/oppia/android/domain/topic/TopicController.kt

      .addAllSubtopic(subtopicList)
      .setTopicPlayAvailability(topicPlayAvailability)
      .build()
  }

+  private fun loadSubtopic(topicId: String, subtopicId: Int): Subtopic {
+    val subtopicRecord = assetRepository.loadProtoFromLocalAssets(
+      assetName = "${topicId}_$subtopicId",


I'm wondering if there's a better way to sent this request rather than the assetname which is a slightly vague variable name since it looks like it's picking up assets for different ids. Maybe asset type (as an enum), asset Id (which is called asset name here) , and base message?

Not sure I follow--what advantage does an enum give us here? The main typing mechanism is the base message type, and introducing an enum may require us to keep the types & enum in-sync which will add complexity.

My suggestion was because we are setting the asset name to be of a different format based on the asset types (i.e. subtopics have a different format of asset name (being topicId_subtopicID) , where as other assets have a different format for their asset names. But it looks like this is because of the way the file names are created, and given that this code may be unnecessary in a few months I guess its okay.

Yeah--I don't think we should over-index on this since the long-term solution should utilize more of a database-type structure for managing files. Thanks for bringing this up, though!

vinitamurthi · 2021-03-24T14:52:31Z

domain/src/main/java/org/oppia/android/domain/topic/TopicListController.kt

+    return if (loadLessonProtosFromAssets) {
+      val topicIdList =
+        assetRepository.loadProtoFromLocalAssets(
+          assetName = "topics",


Here this kind of breaks the paradigm for assetName compared to the fields that are being used above, so I think it may be better to keep assetType, assetId, and baseMessage

It's not a type though, it's the actual file name checked into the APK. Calling it 'assetType' would be misleading. Perhaps 'assetBaseFIilename' would be the most clear, but it's a bit lengthy. I also don't understand how you're distinguishing between 'type' and 'ID'. This is the topic list so it doesn't have an ID.

It could be named better but I'm following existing filenames to keep things a bit simpler. We'll be removing all of this code in coming months, anyway, in favor of downloading & caching completely dynamically.

Same comment as above..given that this code will be dead soon I think its okay

Ack, thanks.

This actually provides Bazel-only support for converting text protos to binary and including those as assets so that we can avoid checking in binary protos to the codebase.

This imports local conversions of all existing dev assets as text proto leveraging the Bazel-only conversion system to convert these to binary protos and include them as assets. This is a necessary step to both eventually replace JSON assets with text proto, and to test that the new proto loading pipeline is working as designed.

Various attempts to fix existing tests + move some of the domain tests over to actually using proto when running with Bazel. App module tests are hanging currently for unknown reasons (only in Bazel).

3.1.0 causes tests to hang indefinitely when run with Bazel.

This moves the coroutine dispatchers to a new threading subpackage. It also simplifies the TestCoroutineDispatcher interface & implementations (which required moving runUntilIdle to CoroutineExecutorServiceTest). The test coroutine annotations were also moved to their own files.

This introduces new Bazel libraries for: - Test dispatchers - Fake system/Oppia clocks - Robolectric dependencies

This also fixes some threading issues in the dispatcher, and clarifies some of its API that was previously unclear (and can lead to subtle race conditions).

This required a bunch of refactoring, adding the first examples of dedicated Bazel test build files, and introducing a new test pattern for sharing code between different implementations. Further, it refined the API for TesCoroutineDispatcher & fixed some issues in the implementations (especially Robolectric). Tests verified as non-flaky in Robolectric across 1000 runs, and 0.2% flaky across 1000 runs for Espresso (though that may be fixed; waiting on a follow-up run to confirm stability).

BenHenning · 2021-04-13T00:02:40Z

Thanks @vinitamurthi. FYI that this is ready for full review--PTAL.

@anandwana001 & @rt4914 PTAL as codeowners.

rt4914

LGTM for code-owner files.

…-local-assets

…oto-lesson-loading-from-local-assets Conflicts: app/src/sharedTest/java/org/oppia/android/app/story/StoryFragmentTest.kt domain/src/test/java/org/oppia/android/domain/exploration/ExplorationDataControllerTest.kt domain/src/test/java/org/oppia/android/domain/topic/TopicControllerTest.kt domain/src/test/java/org/oppia/android/domain/topic/TopicListControllerTest.kt testing/src/test/java/org/oppia/android/testing/story/StoryProgressTestHelperTest.kt

anandwana001

LGTM, nit change but can be ignored

anandwana001 · 2021-04-14T03:28:43Z

domain/domain_assets.bzl

+
+def _extract_proto_sources(deps):
+    """
+    Returns the list of proto source files that make up the specified list of proto dependencies.


we can use the # for comments, nit fix, not blocking

@anandwana001 I don't think we should be. See https://skydoc.bazel.build/docs/writing.html for examples on writing Skydocs. They use multi-line strings, not #. I think we should be treating # as line comments and """ as documentation (which also matches Oppia web).

I did notice I'm not formatting on the correct line, but we can fix that in a later PR by making all documentation consistent.

…oto-lesson-loading-from-local-assets

BenHenning · 2021-04-15T23:14:06Z

ClassroomService Bazel failure will be fixed by #2916.

BenHenning · 2021-04-15T23:38:30Z

Thanks all for the review! Merging this now to keep the PR chain moving forward.

BenHenning added 4 commits March 22, 2021 22:36

Add support for loading proto versions of lessons.

5e33b02

Upgrade Espresso versions to work around build issues.

a3d5f4a

Note that I can't actually verify Espresso tests are working with these versions since the Bazel project doesn't yet have Espresso tests set up.

Merge branch 'fix-protobuf-duplication-issue' into add-support-for-pr…

a017e09

…oto-lesson-loading-from-local-assets

Lint fix post-copying from #2927.

1815e06

BenHenning changed the title ~~Fix #2787: Add support for proto lesson loading from local assets [Blocked: #2977]~~ Fix part of #2787: Add support for proto lesson loading from local assets [Blocked: #2977] Mar 23, 2021

BenHenning changed the base branch from develop to fix-protobuf-duplication-issue March 23, 2021 06:19

BenHenning added 2 commits March 23, 2021 00:35

Remove image loading annotation.

37547c9

This will be added in a later branch in the chain.

Add caching module to needed test suites.

b33ff7a

BenHenning marked this pull request as ready for review March 24, 2021 09:52

BenHenning requested a review from vinitamurthi as a code owner March 24, 2021 09:52

BenHenning marked this pull request as draft March 24, 2021 09:52

vinitamurthi suggested changes Mar 24, 2021

View reviewed changes

BenHenning added 4 commits March 24, 2021 14:31

Add support for importing text protos as assets.

441e2ae

This actually provides Bazel-only support for converting text protos to binary and including those as assets so that we can avoid checking in binary protos to the codebase.

Add gitignore for Android Studio Bazel plugin output.

927508b

Undo Espresso version change since it breaks Bazel tests.

9079ab6

BenHenning mentioned this pull request Mar 25, 2021

Update ExplorationProgressControllerTest to support protos #3007

Closed

BenHenning added 12 commits March 24, 2021 19:53

Fix tests.

be5b785

Various attempts to fix existing tests + move some of the domain tests over to actually using proto when running with Bazel. App module tests are hanging currently for unknown reasons (only in Bazel).

Use correct Espresso core.

34333eb

3.1.0 causes tests to hang indefinitely when run with Bazel.

Refactor testing utilities build graph.

8ab7437

This introduces new Bazel libraries for: - Test dispatchers - Fake system/Oppia clocks - Robolectric dependencies

Introduce dedicated test for TestCoroutineDispatcher.

a78b59d

This also fixes some threading issues in the dispatcher, and clarifies some of its API that was previously unclear (and can lead to subtle race conditions).

Merge branch 'develop' into add-coroutine-dispatcher-tests

797ff85

Workaround ktlint semicolon issue.

b45b212

Lint fixes.

5db9e82

Fix test post-merge.

236a4f0

Merge branch 'develop' into add-coroutine-dispatcher-tests

54dc9bc

Move CoroutineDispatcher to be part of threading.

dc8be26

BenHenning changed the title ~~Fix part of #2787: Add support for proto lesson loading from local assets [Blocked: #2977]~~ [RunAllTests] Fix part of #2787: Add support for proto lesson loading from local assets [Blocked: #2977] Apr 12, 2021

BenHenning changed the base branch from fix-protobuf-duplication-issue to develop April 12, 2021 23:59

BenHenning changed the title ~~[RunAllTests] Fix part of #2787: Add support for proto lesson loading from local assets [Blocked: #2977]~~ [RunAllTests] Fix part of #2787: Add support for proto lesson loading from local assets Apr 13, 2021

Address earlier TODO.

3fae8d3

BenHenning marked this pull request as ready for review April 13, 2021 00:02

BenHenning requested review from anandwana001 and rt4914 as code owners April 13, 2021 00:02

BenHenning assigned rt4914, vinitamurthi and anandwana001 Apr 13, 2021

BenHenning mentioned this pull request Apr 13, 2021

[RunAllTests] Fix #2941: Fix glide image loading in Bazel #2979

Merged

rt4914 approved these changes Apr 13, 2021

View reviewed changes

rt4914 removed their assignment Apr 13, 2021

vinitamurthi approved these changes Apr 13, 2021

View reviewed changes

vinitamurthi removed their assignment Apr 13, 2021

BenHenning added 4 commits April 12, 2021 23:31

Merge branch 'develop' into add-support-for-proto-lesson-loading-from…

522c9b6

…-local-assets

Merge branch 'develop' into add-coroutine-dispatcher-tests

537ec36

Fix broken test build.

4ab42a2

anandwana001 approved these changes Apr 14, 2021

View reviewed changes

anandwana001 assigned BenHenning and unassigned anandwana001 Apr 14, 2021

BenHenning added 2 commits April 15, 2021 14:30

Merge branch 'develop' into add-coroutine-dispatcher-tests

9e99d27

Merge branch 'add-coroutine-dispatcher-tests' into add-support-for-pr…

31f4f9b

…oto-lesson-loading-from-local-assets

BenHenning merged commit cb1ddc8 into develop Apr 15, 2021

BenHenning deleted the add-support-for-proto-lesson-loading-from-local-assets branch April 15, 2021 23:38

BenHenning mentioned this pull request Apr 16, 2021

Fix #2943: Ensure all thumbnails are consistently opaque #2983

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RunAllTests] Fix part of #2787: Add support for proto lesson loading from local assets #2978

[RunAllTests] Fix part of #2787: Add support for proto lesson loading from local assets #2978

BenHenning commented Mar 23, 2021 •

edited

Loading

BenHenning commented Mar 24, 2021 •

edited

Loading

vinitamurthi Mar 24, 2021

BenHenning Apr 12, 2021

vinitamurthi Apr 13, 2021

BenHenning Apr 15, 2021

vinitamurthi Mar 24, 2021

BenHenning Apr 12, 2021

vinitamurthi Apr 13, 2021

BenHenning Apr 15, 2021

BenHenning commented Apr 13, 2021

rt4914 left a comment

anandwana001 left a comment

anandwana001 Apr 14, 2021

BenHenning Apr 15, 2021

BenHenning commented Apr 15, 2021

BenHenning commented Apr 15, 2021

[RunAllTests] Fix part of #2787: Add support for proto lesson loading from local assets #2978

[RunAllTests] Fix part of #2787: Add support for proto lesson loading from local assets #2978

Conversation

BenHenning commented Mar 23, 2021 • edited Loading

BenHenning commented Mar 24, 2021 • edited Loading

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

BenHenning commented Apr 13, 2021

rt4914 left a comment

Choose a reason for hiding this comment

anandwana001 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BenHenning commented Apr 15, 2021

BenHenning commented Apr 15, 2021

BenHenning commented Mar 23, 2021 •

edited

Loading

BenHenning commented Mar 24, 2021 •

edited

Loading