Fix #2134: Fix concept card naming/linking #2980
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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
Also, refactor HtmlParser to better support/handling different tag types & prepare for LaTeX SVG image support.
This will be added in a later branch in the chain.
… into fix-glide-image-loading
|
/cc @rt4914 |
rt4914
reviewed
Mar 24, 2021
| customOppiaTagActionListener?.onConceptCardLinkClicked(view, skillId) | ||
| } | ||
| }, | ||
| 0, text.length, Spannable.SPAN_INCLUSIVE_EXCLUSIVE |
There was a problem hiding this comment.
Suggest adding inline comment
/* start= */ 0, text.length, Spannable.SPAN_INCLUSIVE_EXCLUSIVE
| import org.oppia.android.util.logging.ConsoleLogger | ||
| import org.xml.sax.Attributes | ||
|
|
||
| /** The custom tag corresponding to [MathTagHandler]. */ |
There was a problem hiding this comment.
MathTagHandler file does not exists.
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).
BenHenning
changed the title
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).
|
@rt4914 ready for review now. |
… into fix-glide-image-loading
|
PTAL @anandwana001 as well since some of your codeowners files are affected. |
BenHenning
changed the title
BenHenning
changed the title
|
I plan to take a chance on Bazel for this & the next few PRs to try and get them in a bit faster. It's not ideal but I'm fairly confident the PRs won't introduce issues since Bazel tests were passing for them before. |
|
Also, thanks for the reviews! |
BenHenning
added a commit
that referenced
this pull request
Apr 23, 2021
…#2980] (#2981) * Add support for loading proto versions of lessons. * Upgrade Espresso versions to work around build issues. 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. * Lint fix post-copying from #2927. * Fix Glide in Bazel. * Fix concept card name escaping. Also, refactor HtmlParser to better support/handling different tag types & prepare for LaTeX SVG image support. * Copy over test fix from #2927. * Add support for rendering LaTeX via SVGs. Much of this is copied from #2927 so see that PR's commit history & the PR corresponding to this commit for specific details. * Remove image loading annotation. This will be added in a later branch in the chain. * Add caching module to needed test suites. * Re-add image load annotation for this PR. * Add support for importing text protos as assets. 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. * Import textproto versions of existing dev assets. 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. * Undo Espresso version change since it breaks Bazel tests. * Fix tests. 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. 3.1.0 causes tests to hang indefinitely when run with Bazel. * Move coroutine dispatchers. 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. * Refactor testing utilities build graph. This introduces new Bazel libraries for: - Test dispatchers - Fake system/Oppia clocks - Robolectric dependencies * Introduce dedicated test for TestCoroutineDispatcher. 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). * Add tests for TestCoroutineDispatcher. 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). * Workaround ktlint semicolon issue. * Lint fixes. * Fix test post-merge. * Move CoroutineDispatcher to be part of threading. * Lint fixes. * Undo version upgrade. * Fix broken tests. * Lint fixes. * Add a bit more proto loading test coverage. * Comment fix. * Lint fixes. * Address earlier TODO. * Post-merge restructure. * Post-merge fixes. This removes unneeded dependencies from PersistentCacheStore, including Glide. * Add tests for new tag handlers. Also, add tests for HtmlParser, and clean up some existing tests. * Fix broken test build. * Post-merge lint fixes. * Lint fixes. * Address reviewer comment. * Post-merge fix. * Address pre-merge TODO. * Disable local image loading. Also, add specific LaTeX line in the first fractions interaction so that the new SVG support can be tested. * Fix typo. * Docs + tests. This adds more documentation & context for code added before without it. It also adds a bunch of new testing coverage for changes. The tests are particularly around the html part of the LaTeX loading pipeline, but not the image parts. More of both will be needed specifically for the image loading pipeline changes. * Docs + tests + refactor. This adds documentation for all remaining components introduced in #2981, adds some testing for UrlImageParser, and cleans up the image loading pipeline to be a bit simpler & cleaner (though this will probably break down-stream PRs relying on image transformations). * Lint fixes. * Fix lint failure & re-enable assets.
This was referenced Apr 23, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fix #2134
This fixes an issue with HtmlParser where we weren't correctly unescaping the concept card name/link. Note that this inconsistency may have arisen from the shift to using CustomHtmlContentHandler which partially parses the JSON-escaped custom attribute values (which turns
&"to just").This PR also consolidates all custom tags into using the CustomHtmlContentHandler to avoid global unescaping (e.g. replacing all
&"with empty string) since that's incorrect and breaks encoded JSON objects (one of which is used for math tags).There are also some minor refactors/changes occurring here: