Skip to content
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 part of #4044: Add math tokenizer & parameterized test support #4051

Merged
merged 122 commits into from
Mar 26, 2022

Conversation

BenHenning
Copy link
Member

@BenHenning BenHenning commented Dec 15, 2021

Explanation

Fix part of #4044
Originally copied from #2173 when it was in proof-of-concept form

This PR principally introduces the lexical tokenizer (lexer) for math expressions, as defined by the formal grammar. The tokenizer converts a string into a sequence of Tokens which provide context to the characters available in the string. While the tokenizer is not yet hooked up, it will be leveraged in an upcoming PR to add support for parsing math expressions and equations. Note that special care was taken to ensure the parser is an LL(1) parser to simplify implementation, ease future grammar additions, and to allow for a table-based parsing method in the future if increased performance is necessary.

Some caveats of the tokenizer:

  • It's implemented on top of a sequence with lazy iteration to ensure that no processing time is spent parsing more than necessary (i.e. if the parser short-circuits due to an error then no additional tokenization cost is paid)
  • Due to a limitation in the grammar being LL(1), function name parsing is a bit more nuanced and limiting. The lexer has to assume that the start of a function name is actually a name and not a series of variables. To account for this, a special token exists for invalid function names. See the tokenizer's test suite for more details on valid and invalid character combinations.
  • Care is taken to ensure that overflowed integers & doubles result in failing tokens
  • The lexer takes on a bit of the actual parsing (for integers & reals) to simplify the parser, but the majority of parsing is not done in the lexer

This PR introduces a new Truth subject for the tokenizer's Token class, though similar to subjects introduced in previous PRs this one does not include tests. #4121 is tracking adding them in the future.

To enforce the LL(1) grammar, the tokenizer makes use of a PeekableIterator that doesn't allow for lookahead beyond one character. This iterator will also be used by the upcoming parser to keep the LL(1) property.

To enable better testing for the tokenizer, I decided to add parameterized tests. Unfortunately, there wasn't an obvious way on how to exactly do this. While parameterized JUnit tests work, there are some limitations:

  • They won't work with instrumentation or Robolectric
  • They don't support combining parameterized & non-parameterized tests

Instead, I decided to implement a custom parameterized test runner that addresses both of the points above. I used AndroidJUnit4 and ParameterizedRobolectricTestRunner as key references into understanding how to actually build this. While the implementation we're now using is quite different than both, they serve as parts of the basis. AndroidJUnit4 is final, so we needed to implement a similar switching mechanism to select Robolectric or Espresso test helpers based on the platform (this is controlled via a test class-level annotation). Further, Robolectric's runner was important to understand how to even set up a parameterized runner, and to better understand how to make Robolectric work in this environment.

The result seems to be a very clean test runner, but I'm keen to hear reviewer's thoughts on this. Note a couple things:

  • While there's an Espresso-supported runner, I didn't actually verify that it works since we haven't yet gotten instrumentation tests working natively with Bazel (though I do expect that it'll work)
  • I didn't test this on Gradle, but per the CI run it appears to work just fine
  • There are no tests for any of the new runners or their utilities. The utilities are fairly trivial, and the runners are difficult to test. I decided not to test them or file a tracking issue as they'll likely be the last components we add tests for when we aim to raise the codebase's code coverage (I didn't feel the difficulty was worth overcoming here over vs. manual verification).
  • The runner has very robust error detection to try and reduce potential errors, including enforcing parameter order consistency
  • I ran into a little snag with ktlint--see Update ktlint #4122
  • The previous iteration of this led to platform selection between Espresso & Robolectric based on Bazel dependencies, but I decided to change this to be an explicit annotation. This has a number of considerations:
    • It leads to automatic build fixes (i.e. you can't compile a reference to a class without the Bazel dependency being complete)
    • It allowed me to introduce a JUnit-specific test suite (after changing MathTokenizerTest to this, I saw an almost 40x decrease in runtime; I plan to use this for all future parameterized math tests since Robolectric isn't actually needed for these and is much slower)
    • It doesn't facilitate shared tests like AndroidJUnit4, but we could conceivably create a shared runner in the future that uses either the instrumentation or Robolectric runner based on the current platform (similar to how AndroidJUnit4 works--see the commit history for the old code that did this)
  • Finally, see the KDoc for OppiaParameterizedTestRunner for much more details on the runner, including both a code example and suggestions on when to actually use the runner. Further, MathTokenizerTest demonstrates real uses of the runner.

The way that the runner works is it generates a test suite that combines one runner for all non-parameterized tests with a runner for each parameterized iteration (so the total number of tests in a suite is number_of_non_parameterized_tests + sum(parameterized_iterations_in_all_parameterized_tests). The iteration names are appended to parameterized test's names to provide both Bazel filtering support and direct error reporting when tests fail (so that the exact iteration is known & can be run/debug in isolation). Android Studio will run all iterations when filtering on a method (since that should match against all), but this might be environment-specific (I verified this with Bazel in Android Studio, but not with Gradle).

Script asset changes

Test file exemptions were added for the new JUnit rules & the TokenSubject class--see above for the rationale.

Regex content exemptions were added for ParameterizedMethod since it uses Locale and capitalize. It doesn't have access to MachineLocale (at least, currently), and is only running in tests (so localization correctness isn't as important). Finally, the chosen locale to use for capitalization is US which should more or less match field names in tests for the Oppia codebase.

Further, a new regex check was added to require all uses of the new test runner to require approval (i.e. by adding an exemption). This will help ensure it doesn't get abused/used too broadly since parameterization should be an exceptional case rather than the norm.

Essential Checklist

  • The PR title and explanation each start with "Fix #bugnum: " (If this PR fixes part of an issue, prefix the title with "Fix part of #bugnum: ...".)
  • Any changes to scripts/assets files have their rationale included in the PR explanation.
  • The PR follows the style guide.
  • The PR does not contain any unnecessary code changes from Android Studio (reference).
  • The PR is made from a branch that's not called "develop" and is up-to-date with "develop".
  • The PR is assigned to the appropriate reviewers (reference).

For UI-specific PRs only

N/A -- This PR introduces a non-user facing utility, and the utility isn't yet hooked up (it will be in a subsequent PR).

Also adds testing libraries for both + fractions & reals (new
structure).

Most of this is copied from #2173.
…tor' into add-support-for-math-expressions-pt3-math-expression-protos
…otos' into add-support-for-math-expressions-pt4-commutative-comparisons-protos
…isons-protos' into add-support-for-math-expressions-pt5-polynomial-protos
This is mostly copied from #2173.
…tor' into add-support-for-math-expressions-pt3-math-expression-protos
…otos' into add-support-for-math-expressions-pt4-commutative-comparisons-protos
…isons-protos' into add-support-for-math-expressions-pt5-polynomial-protos
… into add-support-for-math-expressions-pt6-tokenizer
@BenHenning
Copy link
Member Author

Hi. As of today, some main reviewers have taken time off for the next few weeks, so it may take a little while before we can look at this PR. We appreciate your patience while some of our team members recharge. We'll be fully returning on 4 January 2021.

@oppiabot
Copy link

oppiabot bot commented Dec 24, 2021

Hi @BenHenning, I'm going to mark this PR as stale because it hasn't had any updates for 7 days. If no further activity occurs within 7 days, it will be automatically closed so that others can take up the issue.
If you are still working on this PR, please make a follow-up commit within 3 days (and submit it for review, if applicable). Please also let us know if you are stuck so we can help you!

@oppiabot oppiabot bot added the stale Corresponds to items that haven't seen a recent update and may be automatically closed. label Dec 24, 2021
@oppiabot oppiabot bot closed this Dec 31, 2021
@BenHenning BenHenning reopened this Jan 4, 2022
@oppiabot oppiabot bot removed the stale Corresponds to items that haven't seen a recent update and may be automatically closed. label Jan 4, 2022
@oppiabot
Copy link

oppiabot bot commented Jan 11, 2022

Hi @BenHenning, I'm going to mark this PR as stale because it hasn't had any updates for 7 days. If no further activity occurs within 7 days, it will be automatically closed so that others can take up the issue.
If you are still working on this PR, please make a follow-up commit within 3 days (and submit it for review, if applicable). Please also let us know if you are stuck so we can help you!

@oppiabot oppiabot bot added the stale Corresponds to items that haven't seen a recent update and may be automatically closed. label Jan 11, 2022
This also changes regex handling in the check to be more generic for
better flexibility when matching files.
… into add-support-for-math-expressions-pt2-math-utility-refactor

Conflicts:
	app/src/test/java/org/oppia/android/app/parser/StringToFractionParserTest.kt
	utility/src/main/java/org/oppia/android/util/math/FractionParser.kt

Includes post-merge fixes.
…tor' into add-support-for-math-expressions-pt3-math-expression-protos
…otos' into add-support-for-math-expressions-pt4-commutative-comparisons-protos
…isons-protos' into add-support-for-math-expressions-pt5-polynomial-protos
… into add-support-for-math-expressions-pt6-tokenizer
@oppiabot oppiabot bot removed the stale Corresponds to items that haven't seen a recent update and may be automatically closed. label Mar 18, 2022
@oppiabot
Copy link

oppiabot bot commented Mar 25, 2022

Hi @BenHenning, I'm going to mark this PR as stale because it hasn't had any updates for 7 days. If no further activity occurs within 7 days, it will be automatically closed so that others can take up the issue.
If you are still working on this PR, please make a follow-up commit within 3 days (and submit it for review, if applicable). Please also let us know if you are stuck so we can help you!

@oppiabot oppiabot bot added the stale Corresponds to items that haven't seen a recent update and may be automatically closed. label Mar 25, 2022
@oppiabot oppiabot bot removed the stale Corresponds to items that haven't seen a recent update and may be automatically closed. label Mar 25, 2022
…tor' into add-support-for-math-expressions-pt3-math-expression-protos
…otos' into add-support-for-math-expressions-pt4-commutative-comparisons-protos
…isons-protos' into add-support-for-math-expressions-pt5-polynomial-protos
Base automatically changed from add-support-for-math-expressions-pt5-polynomial-protos to develop March 26, 2022 03:42
@BenHenning BenHenning requested a review from rt4914 as a code owner March 26, 2022 03:42
@BenHenning BenHenning changed the title Fix part of #4044: Add math tokenizer & parameterized test support [Blocked: #4050] Fix part of #4044: Add math tokenizer & parameterized test support Mar 26, 2022
@BenHenning BenHenning merged commit b14c3b6 into develop Mar 26, 2022
@BenHenning BenHenning deleted the add-support-for-math-expressions-pt6-tokenizer branch March 26, 2022 04:38
BenHenning added a commit that referenced this pull request Mar 27, 2022
…ns (#2173)

## Explanation
Fixes #4044

This PR introduces the UI interaction views for numeric expressions, algebraic expressions, and algebraic equations, finishing the initial implementation of the broader math expressions project. This work unblocks adding support for 4 additional topics in the app: Multiplication (support was lost after Alpha MR2 due to a change that added numeric expression questions), Addition & Subtraction, Division, and Expressions & Equations.

This PR finishes a long chain of PRs that were needed to provide the domain functionality to support these new interactions, as a significant amount of mathematics functionality needed to be added including:
- Support for representing generic math expressions/equations, polynomials, and reals
- Support for parsing math expressions
- Support for converting between math expressions and:
  - LaTeX (for rendering)
  - Human-readable sentences (for accessibility)
  - Polynomials and real-value evaluation (for equivalence checking)
  - Comparable structures to verify that two expressions/equations match irrespective of associativity or commutativity order
- Robust error detection to support nearly 2 dozen special cased errors with potential for countless more if needed to ensure users receive excellent feedback when inputting expressions

All of the above also required thorough testing to ensure correctness. See the previous PRs corresponding to #4044 for the full context, and thorough PR descriptions explaining past changes.

From a UI perspective, this PR is introducing:
- The new interaction views and relevant view models, including the logic for constructing and displaying errors for invalid answers
- Support for playing a custom content description for math expression answers so that screenreader users can better understand the expression they've entered
- Support for rendering the LaTeX representation of entered expressions
- A new developer options menu to provide support to input arbitrary expressions and equations and convert them to different outputs to test the underlying math engine that's been built (though it can't yet test the classifiers, but it provides enough information to verify the more complex bits behind the classifiers)

From a functional perspective, this PR is introducing:
- Support for loading JSON explorations that include math expressions
- A new test exploration that's been added to test topic 0 to demonstrate all three interactions and each of those interaction's classifiers (9 total states have been added)

The above, and more, are explained in more detail below.

### Background on UI implementation for new interactions

The UI implementation leverages a single new interaction view & model for all three new interaction types since they are very similar to one another, including handling accessibility and LaTeX rendering cases. The customization arguments, however, don't exactly match so there are some inconsistencies there.

### Background on parsing approach
The new interactions rely on custom math infrastructure for parsing math expressions since no suitable libraries were found that were both license compatible and didn't leverage native code. While this significantly increases the scope of code that needs to be maintained in the project, it has the added benefit of leveraging extremely specific functionality to reduce inconsistencies with Oppia web and ensure a very high quality learner experience when it comes to answer classification and error detection.

Expressions are constructed using an LL(1) recursive-descent parser with early-exit error detection (see the [technical specification](https://docs.google.com/document/d/1JMpbjqRqdEpye67HvDoqBo_rtScY9oEaB7SwKBBspss/edit)). Parsing first goes through an LL(1) lazy tokenization process before being parsed lazily into proto-defined math expression/equation structures. Classifier needs are satisified through custom infrastructure that can evaluate numeric expressions to a single real value, compare math expressions/equations with floating point error correction, compare math expressions/equations irrespective of operation associativity and commutativity by transforming it to an intermediary representation, and can compare expressions/equations for equivalence by first converting it to a polynomial using a custom nearly fully feature complete polynomial system. Finally, UI-specific needs are satisified through generators for both LaTeX and human-readable accessibility strings.

The preceding PRs in this project contain the implementations for parsing, conversion, and comparsion and include PR descriptions that explain these systems & other peripheral changes in great detail.

### PR history
This PR contains **all** of the PRs corresponding to both the previous attempt at implementing math expressions, and the latest (since all commits end up being duplicated in PR chains). There's also some early work that preceded the original work in this PR that's included in its history (it goes back quite a bit).

For reference, here's the entire chain of PRs in order that precede this one for implementing math expressions support:
1. #4045
2. #4046
3. #4047
4. #4049
5. #4050
6. #4051
7. #4052
8. #4054
9. #4055
10. #4056
11. #4057
12. #4058
13. #4059
14. #4061
15. #4063
16. #4068
17. #4237
18. #4239

### Refactors, miscellaneous changes, and future work
All interaction view models were refactored to use a factory pattern rather than a function to improve readability, and to make that view model construction consistent with other similar situations (i.e. cases where new instances need to be created with Dagger dependencies).

Split screen supported interaction IDs have been refactored to be a Dagger set to avoid a direct dependency on InteractionViewModelModule to access the IDs (the new solution is more Dagger 2 idiomatic).

This PR doesn't finish all aspects of the project as there are a number of improvements that have been proposed toward the end of development. These have been cataloged in #4254 and will be implemented in a follow-up PR (but will only be started after this PR & its predecessors have been merged). None of those work items block the overall project, and are intentionally being delayed to a future work item to ensure that math expressions can land more quickly.

### Test specifics & exemptions
Changes were needed in ``EditTextInputAction`` to support inputting Unicode text (which is necessary for some UI tests that verify using math symbols such as for times and divide). Espresso itself doesn't support inputting these characters (presumably since it can't easily find them on the keyboard), so the action now exposes an option to directly set the text.

Exemption explanations:
- ``MathExpressionInteractionsViewTest``: this has been enabled to allow parameterized tests to ensure it can verify a broad set of questions and answers. This might actually be a nice pattern to follow for other interaction tests in the future.
- All test exemptions are either can't be obviously tested (listeners & annotations), are tested through other suites (view models & presenters), is especially difficult to test (``ParameterizedAutoAndroidTestRunner``), or is deemed not important enough to add tests for due to a trivial implementation and in the effort to finish this project sooner (``SplitScreenInteractionModule``).

Note that the defaulting cases for accessibility strings & LaTeX (i.e. the scenarios where generation to either fails) can't easily be tested since the scenarios in which generation fails arise from invalid answers that can't be submitted since they trigger submit-time errors. However, the failure cases for both of these utilities have been thoroughly checked in their respective test suites.

This PR also introduces a new addition to ``OppiaParameterizedTestRunner``: ``ParameterizedAutoAndroidTestRunner``. This runner acts like ``AndroidJUnit4`` wherein it will automatically select a Robolectric or Espresso runner depending on the current runtime environment. This ought to only be used by shared tests that are supposed to run on both platforms, otherwise Robolectric or Espresso specific runners should be used (or JUnit for non-Robolectric tests).

## Essential Checklist
- [x] The PR title and explanation each start with "Fix #bugnum: " (If this PR fixes part of an issue, prefix the title with "Fix part of #bugnum: ...".)
- [x] Any changes to [scripts/assets](https://github.com/oppia/oppia-android/tree/develop/scripts/assets) files have their rationale included in the PR explanation.
- [x] The PR follows the [style guide](https://github.com/oppia/oppia-android/wiki/Coding-style-guide).
- [x] The PR does not contain any unnecessary code changes from Android Studio ([reference](https://github.com/oppia/oppia-android/wiki/Guidance-on-submitting-a-PR#undo-unnecessary-changes)).
- [x] The PR is made from a branch that's **not** called "develop" and is up-to-date with "develop".
- [x] The PR is **assigned** to the appropriate reviewers ([reference](https://github.com/oppia/oppia-android/wiki/Guidance-on-submitting-a-PR#clarification-regarding-assignees-and-reviewers-section)).

## For UI-specific PRs only
I've verified the interaction with the accessibility scanner locally and it didn't mention any new suggestions (there was one suggestion on the label for the input box when inputting something, and another for the congratulations banner contrast--both of which are existing issues that affect other interactions).

### Screenshots of new UIs
| Device type | Language | Orientation | Input box | Error | Inputted answer |
|-------------|----------|-------------|-----------|-----------------|-------|
| Handset     | English  | Portrait    | ![image](https://user-images.githubusercontent.com/12983742/158960886-b66b5f59-48e9-4005-a05d-60cdc9917cee.png)      | ![image](https://user-images.githubusercontent.com/12983742/158966282-108719be-44cb-42ab-8570-fe2f4dd49cfe.png) | ![image](https://user-images.githubusercontent.com/12983742/158966035-c1db8ee1-01a8-4e2f-bab7-c9622965c5b5.png)        |
| Handset     | English  | Landscape   | ![image](https://user-images.githubusercontent.com/12983742/158966436-ef289ea9-2b69-4bd8-b07a-8e0ac917f30a.png)      | ![image](https://user-images.githubusercontent.com/12983742/158966626-8a19d38c-2cbe-4544-aab1-57da419ca405.png)            | ![image](https://user-images.githubusercontent.com/12983742/158966701-768af358-912c-44c4-8c20-e82db23b39a7.png)  |
| Handset     | Arabic   | Portrait    |  ![image](https://user-images.githubusercontent.com/12983742/158967405-29461acf-d160-46e2-bbaa-233a97b99f91.png)     | ![image](https://user-images.githubusercontent.com/12983742/158967611-d3494512-ff49-4c3e-988d-3c2a0eb48943.png)            | ![image](https://user-images.githubusercontent.com/12983742/158969001-a5d516bb-7db2-49f3-b614-baf298c0ce95.png)  |
| Handset     | Arabic   | Landscape   | ![image](https://user-images.githubusercontent.com/12983742/158969671-f2f92523-6045-4908-9b3a-c0efec525e55.png)      | ![image](https://user-images.githubusercontent.com/12983742/158970091-ba858e7e-deab-4189-b06a-91208049ab79.png)            | ![image](https://user-images.githubusercontent.com/12983742/158971054-c64adfbd-06a1-4cc7-a3df-983793e8bd30.png)  |
| Tablet      | English  | Portrait    | ![image](https://user-images.githubusercontent.com/12983742/158975533-046a1d31-145a-4671-b212-6770f0b09b4a.png)      | ![image](https://user-images.githubusercontent.com/12983742/158975620-e404da29-630b-4a19-be8b-13ff5b853601.png)            | ![image](https://user-images.githubusercontent.com/12983742/158975673-a20f6ddb-1669-4035-a819-c23eccee666b.png)  |
| Tablet      | English  | Landscape   | ![image](https://user-images.githubusercontent.com/12983742/158975794-c36f7bee-4f47-4683-b3a2-7e012ea7039c.png)      | ![image](https://user-images.githubusercontent.com/12983742/158975835-a43957f8-c831-4fe3-a69e-3df7b6be3013.png)            | ![image](https://user-images.githubusercontent.com/12983742/158975869-59a7b989-83c1-4162-9df6-6d07f79c5005.png)  |
| Tablet      | Arabic   | Portrait    |  ![image](https://user-images.githubusercontent.com/12983742/158976398-55014bf8-0146-4507-95a3-733fab00f36f.png)     | ![image](https://user-images.githubusercontent.com/12983742/158976477-0736e4c3-81c0-42fd-8160-6ba0cb528e44.png)            | ![image](https://user-images.githubusercontent.com/12983742/158976579-b0a834fc-793e-4205-a2ad-c4029665430d.png)  |
| Tablet      | Arabic   | Landscape   | ![image](https://user-images.githubusercontent.com/12983742/158976744-7c2fb78b-7338-4688-94a0-afa65126b972.png)      | ![image](https://user-images.githubusercontent.com/12983742/158976832-43aee849-38fe-4fb7-8ebf-af13e0cb5cb0.png)            | ![image](https://user-images.githubusercontent.com/12983742/158976882-4e309290-63ab-4ea3-a3a6-39f64615afe4.png)  |

Note that the interaction's errors haven't yet been translated. Also, some work may need to be done in the future to ensure directionality makes sense both for input and rendering (asking folks yielded that input is typically right-to-left but math expressions are still represented left-to-right, so it's not clear if math input should also be left-to-right). In the case above, the text view actually switches the '+2' to '2+1' once it has another number since the context is more complete. It's possible RTL users are already used to quirks like this. It's also likely the custom math keyboard would be a good means of solving this issue long-term.

Screenshot of the new developer options menu for testing math expressions/equations:
![math_expressions_debug_menu](https://user-images.githubusercontent.com/12983742/158933786-9654c85e-b7b9-49fa-8e5f-8d37e598139f.png)

Note that I didn't bother to screenshot other configurations for the developer options menu since it's only visible to developers, so it's less important to make sure it meets the accessibility, language, device, and orientation requirements that the other screens of the UI are held to.

### Videos
| Flow being demonstrated                                    | Video recording |
|------------------------------------------------------------|-----------------|
| Submitting numeric expressions answers                     | https://user-images.githubusercontent.com/12983742/158933258-74992080-a9ec-46da-b752-c00b406ab3ec.mp4            |
| Submitting algebraic expressions answers                   | https://user-images.githubusercontent.com/12983742/158933296-0ea70704-9e3b-4e0d-9150-680a47f9ece2.mp4            |
| Submitting answers that cause errors                           | https://user-images.githubusercontent.com/12983742/158933391-602356c3-ccad-4199-8c0b-47d42abd7896.mp4            |
| Rendering fractions vs. divisions                          | https://user-images.githubusercontent.com/12983742/158933425-9abc640b-289e-484f-8f30-dab38789c99e.mp4            |
| Submitting numeric expressions with a screenreader enabled | https://user-images.githubusercontent.com/12983742/158933446-21afbcd6-0ea8-4558-8c68-9fa0157a7e3d.mp4            |
| Submitted math equations with a screenreader enabled       | https://user-images.githubusercontent.com/12983742/158933460-ddf39ba3-1fd8-4bfc-82e9-cc1845846646.mp4            |

### Espresso test results
| ![final_math_pr_developeroptionsactivitytest_passing](https://user-images.githubusercontent.com/12983742/158932650-4c55fd70-0c58-4972-b1c9-dda727190f9a.png) | ![final_math_pr_developeroptionsfragmenttest_passing](https://user-images.githubusercontent.com/12983742/158932667-9ab52468-6c7d-4088-8e80-3e71afe3cc29.png) |
|------|------|
| ![final_math_pr_htmlparsertest_passing](https://user-images.githubusercontent.com/12983742/158932678-aadd0ba5-f7c4-49e0-b7a8-0cf0734a269d.png) | ![final_math_pr_mathexpressioninteractionsviewtest_passing](https://user-images.githubusercontent.com/12983742/158932687-e1b5df4a-9a5e-4f5e-a0bb-0b1e60ce562e.png) |
| ![final_math_pr_mathexpressionparseractivitytest_passing](https://user-images.githubusercontent.com/12983742/158932699-e71b0b5a-f4c6-475c-9cc1-db626de9ed51.png) | ![final_math_pr_mathexpressionparserfragmenttest_passing](https://user-images.githubusercontent.com/12983742/158932712-a68cee27-4a2f-49ab-895b-f2d021cbc3d8.png) |
| ![final_math_pr_questionplayeractivitytest_passing](https://user-images.githubusercontent.com/12983742/158932734-e9aefddf-13e9-4cf6-9a22-37588ca527e7.png) | ![final_math_pr_statefragmenttest_passing](https://user-images.githubusercontent.com/12983742/158932752-3bc3d821-b703-46a9-a5ef-29fd568d896f.png) |

Note that in the case of ``MathExpressionInteractionsViewTest`` this is the first PR that demonstrates using parameterized tests with Espresso.

Commit history:

* Treat en-dash as a subtraction symbol.

* Add explicit platform selection for paramerized.

This adds explicit platform selection support rather than it being
automatic based on deps. While less flexible for shared tests, this
offers better control for tests that don't want to to use Robolectric
for local tests.

This also adds a JUnit-only test runner, and updates MathTokenizerTest
to use it (which led to an almost 40x decrease in runtime).

* Exemption fixes.

Also, fix name for the AndroidJUnit4 runner.

* Remove failing test.

* Fix unary expression precedence.

Also, use ParameterizedJunitTestRunner for MathExpressionParserTest.

* Fixes & add more test cases.

* Post-merge fixes & test changes.

Also, update RealExtensionsTest to use the faster JUnit runner.

* Use utility directly in LaTeX tests.

* Post-merge fixes.

Also, update ExpressionToComparableOperationConverterTest to use the
fast JUnit-only runner.

* Post-merge fixes.

Also, update PolynomialExtensionsTest to use fast JUnit-only runner.

* Post-merge fixes.

Also, update float interval per new tests.

* Lint & other check fixes.

* Replace deprecated term.

* Post-merge fixes.

* Add full test suites for alg exp classifiers.

* Lint & static check fixes.

* Fix test on Gradle.

* Fix test for Gradle.

* Add tests for math equations.

And, post-merge fixes.

* Static check & lint fixes.

* Post-merge fixes.

Verified CI checks & all unit tests are passing.

* Split up tests.

Also, adds dedicated BUILD.bazel file for new test.

* Add missing test in Bazel, and fix it.

* Correct order for genrule.

* Add full test suite.

* Clean up + KDocs + exemption.

* Lint fixes.

* Post-merge fix.

* Cache KotliTeX renders.

Directly rendering LaTeX through KotliTeX is way too slow, so this
introduces a custom flow through Glide that computes a PNG for the LaTeX
on a background thread and then caches it as part of Glide's cache to
speed up re-renders of the LaTeX. We may need to manage/prune the cache
over time, but for now we'll just rely on Glide's internal behaviors.

This also documents some new tests that should be added, but it's not
comprehensive.

* Add tests, docs, and exemptions.

* Update to fixed version of KotliTeX.

The newer version correctly computes the bounds for rendered LaTeX.

* Lint fixes.

* Add new dependency licenses.

This isn't done yet (some of the licenses still need to be fixed).

* Fix license links.

Note that the kxml one was tricky since its Maven entry says it's
licensed under BSD and CC0 1.0, and its SourceForge link says the same
plus LGPL 2.0. However, the source code and GitHub version of the
project license it under MIT, and this seems to predate the others so
it seems like the most correct license to use in this case and the one
that we're using to represent the dependency.

* Fix Gradle build.

This uses a version of KotliTeX that builds correctly on Jitpack for Gradle,
and fixes the StaticLayout creation to use an alignment constant that
builds on Gradle (I'm not sure why there's a difference here between
Gradle & Bazel, but the previous constant isn't part of the enum per
official Android docs).

* Create the math drawable synchronously.

This requires exposing new injectors broadly in the app since the math
model loader doesn't have access to the dependency injection graph
directly.

* Remove new deps from Maven list.

They were incorrectly pulled in by KotliTeX.

* Add argument partitioning.

This fixes cases where argument calls may be very large and fail to
execute due to exceeding system limitations.

* Post-merge fixes.

Note that only the following tests are failing (some of which may be due
to the previous branch):
- MarkChaptersCompletedFragmentTest
- MarkStoriesCompletedFragmentTest
- StoryProgressTestHelperTest
- ModifyLessonProgressControllerTest
- TopicListControllerTest
- ComputeAffectedTestsTest
- MarkTopicsCompletedFragmentTest
- HomeActivityTest

* Make allowance for empty cases to fix tests.

These tests correspond to real scenarios.

* Lint fixes.

* Fix broken tests and add reasonable exemptions.

* First pass at adding tests.

Some debugging left, and more tests to add.

This also adds auto-switching support for parameterized tests.

* Fix DeveloperOptionsActivityTest.

Also, fix all other test builds (for Gradle). This will probably require
some reformatting.

* Add & fix remaining tests.

This also fixes a bug where the correct answer a11y label was being
incorrectly assumed to be the correct answer even for incorrect cases.

* Add docs & clean up remaining non-lint parts.

* Lint & small breakage fixes.

* Fix broken tests.

* Address reviewer comment.

Clarifies the documentation in the test runner around parameter
injection.

* Fix broken build.

* Fix broken build post-merge.

* Fix broken post-merge classifier.

* Address reviewer comment.

* Post-merge build fixes.

* Post-merge build fixes for new classifiers.

* Post-merge build fixes.

* Correct reference document link.

* Ensure LaTeX isn't stretched or cut-off.

The comments in-code have much more specifics on the approach.

* Add and fix missing test (was broken on Gradle).

* Fix Gradle tests.

This commit has verified the following tests now pass on Espresso:
- MathExpressionInteractionsViewTest
- HtmlParserTest
- MathExpressionParserActivityTest
- MathExpressionParserFragmentTest
- DeveloperOptionsActivityTest
- DeveloperOptionsFragmentTest

StateFragmentTest is having issues (it ends up hanging), so further
investigation is needed.

* Update to newer version of Kotlin coroutines.

This version includes StateFlow which will be a really useful mechanism
for helping to avoid using critical sections.

* First attempt to fix deadlock.

This uses StateFlows and an actor to avoid the deadlock.

This is missing necessary hint changes that are coming in a later
commit. Tests currently fail, and questions haven't yet been migrated
(and won't until the fixes are verified).

* Attempt to make hint handler multi-threadable.

This is a midway solution that's being committed for posterity, but will
be reverted in favor of a solution that forces hint handler to be unsafe
across multiple threads (since it's much simpler, and works given that
all users of it now synchronize state management using an actor).

* Finish fixing state player.

This includes a fix for accessibility string handling (since the new
flow triggers one of these to fail). It also adds a Bazel file for
StateFragmentTest (I spent some time trying to get Espresso tests
working with Bazel but ran into a compatibility issue).

StateFragmentTest has been verified to pass on Robolectric and Espresso
with this change.

This sets up the project for fixing questions in the next commit.

* First pass on migrating question controller.

This also includes a migration for exploration & question domain tests
to use the test monitor utility.

The question tests currently fail since there's a bug in AsyncResult
where it won't support null values during transformations.

* Refactor AsyncResult into a sealed class.

This also introduces an AsyncResultSubject, and more or less fully fixes
issue #3813 in all tests.

* Refactor AsyncResult into a sealed class.

This also introduces an AsyncResultSubject, and more or less fully fixes
issue #3813 in all tests.

This is a cherry-pick from the fix-progress-controller-deadlock branch
since it ended up being quite large (it made more sense to split it into
a pre-requisite PR).

Conflicts:
	app/src/main/java/org/oppia/android/app/player/state/testing/StateFragmentTestActivityPresenter.kt
	domain/src/main/java/org/oppia/android/domain/exploration/ExplorationProgressController.kt
	domain/src/main/java/org/oppia/android/domain/question/QuestionAssessmentProgressController.kt
	domain/src/test/java/org/oppia/android/domain/exploration/lightweightcheckpointing/BUILD.bazel
	testing/src/main/java/org/oppia/android/testing/data/DataProviderTestMonitor.kt
	utility/src/main/java/org/oppia/android/util/data/DataProviders.kt

* Post-merge fixes and updates for consistency.

* Post-merge fixes.

* TODO has been addressed.

* Fix documentation & add tests.

* Lint fixes.

* Lint & post-merge fixes.

Questions-related tests still fail and need to be fixed now that
AsyncResult has been refactored to support null values.

* Post-merge test fixes.

The core affected UI/domain tests have now been verified as working on
Robolectric. The full test suite is next.

* Add documentation & tests.

Also, fix a bug in the automatic StateFlow DataProviders wherein they
wouldn't notify correctly on changes. This led to some simplifications
in the exploration & question progress controllers.

* Lint fixes.

* Fix gradle tests.

I've verified in this commit that all Gradle tests build & run locally
(at least on Robolectric).

* Fix Proguard build.

This required bringing kotlinx-coroutines-android up-to-date with
kotlinx-coroutines-core (since that was updated on this branch).

New but reasonable Proguard warning exemptions also needed to be added.

* Post-merge fixes.

* Gradle fixes.

This also fixes an issue wherein answers couldn't be resubmitted for
math expression interactions after an error occurred.

* Fix parameterized runners for Espresso.

This fixes a typo when loading the AndroidJUnit4 runner during
parameterization runer selection, and ensures that test names don't
contain illegal characters that would break the runner.

* Post-merge fix.

* More post-merge fixes.

* Fix TODO comment.

* Post-merge lint fixes.

* Post-merge fix.

* Fix exploration routing issue.

The underlying problem was that the PR inadvertently changed the
behavior of comparing two results wherein results of different times
would be considered different and be re-delivered (which happened to
break exploration routing, and likely a variety of other places).

This introduces a new method for checking equivelance rather than
confusingly assuming that users of AsyncResult don't care about the
result's time during comparison checking.

New tests have been added to verify the functionality works as expected
(and fails when expected), and I manually verified that the exploration
routing issue was fixed. More details on the specific user-facing issue
will be added to the PR as a comment.

* Post-merge fixes.

* Post-merge fixes.

The local repo has been verified to pass nearly all static analysis and
lint tests, as well as all Robolectric tests. The developer and alpha
versions of the app build.

* Update KotliTeX version.

This version doesn't have debug drawing enabled.

* Fix lifecycle breakage.

I noticed downstream that quickly navigating back and forth to enter &
exit an exploration can cause the progress controller to get into a bad
state that either leads to a broken experience (stacked explorations or a
blank state), or a crash.

The issue was coming from DataProviders being shared across sessions
even though the underlying state was different. This change ensures that
providers stay completely independent, similar to the core session
state.

* Address reviewer comment.

* Update play session controllers.

This ensures that the command queue itself is fully isolated to avoid
delayed events potentially leaking across sessions (now that the session
ID barrier has been removed).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants