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.

Sign up for GitHub

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

Jump to bottom

Add tests for MathParsingErrorSubject #4132

Open
BenHenning opened this issue Jan 24, 2022 · 0 comments
Open

Add tests for MathParsingErrorSubject #4132

BenHenning opened this issue Jan 24, 2022 · 0 comments
Labels
enhancement End user-perceivable enhancements. Impact: Low Low perceived user impact (e.g. edge cases). Issue: Needs Clarification Indicates that an issue needs more detail in order to be able to be acted upon. Work: Medium The means to find the solution is clear, but it isn't at good-first-issue level yet. Z-ibt Temporary label for Ben to keep track of issues he's triaged.

Comments

@BenHenning
Copy link
Member

#4052 introduces a new custom Truth test subject: MathParsingErrorSubject. It would be advantageous to add thorough tests for this to ensure that:

  • Each of the subject methods correctly allow users to test specific properties
  • Each of the subject methods are verified to fail when expected with the correct failure reason being verified

See #4097 for a similar issue.
@BenHenning BenHenning mentioned this issue Jan 24, 2022
Merged
6 tasks
BenHenning added a commit that referenced this issue Mar 26, 2022
@BenHenning
Fix part of #4044: Add math expression/equation parser (with error de…
91f2ce9 
…tection) (#4052)

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

This PR introduces an LL(1) recursive descent parser for numeric expressions, algebraic expressions, and algebraic equations (per [this grammar specification](https://docs.google.com/document/d/1JMpbjqRqdEpye67HvDoqBo_rtScY9oEaB7SwKBBspss/edit#bookmark=id.wtmim9gp20a6)).

Some details about the implementation:
- Error handling is managed through a MathParsingResult sealed class with corresponding transformations and errors specifically tied to MathParsingError. I actually quite like this pattern since it avoids relying on exceptions yet still provides as much context as the frontend needs (this will be more apparent in a future PR that leverages these errors for user-visible errors). I might try to adopt this as the more broad error handling pattern in the future.
- Error detection is split into two levels of strictness: required-only (i.e. irrecoverable) and all errors. 'Optional' errors are ones that can still be represented by the ``MathExpression`` proto, but are likely learner errors. While classifiers will never disable these, tests do in order to test certain scenarios that are possible per the grammar.
- The parser implementation is intentionally generic so that both numeric & algebraic expressions can share the same parser (for simplicity)
- A specific effort is made to force LL(1) parsing for simpler grammar & implementation and long-term maintainability (since it's likely this grammar will need to be changed in the future). This is done by leveraging an LL(1) tokenizer and wrapping the ``Sequence`` provided by the tokenizer in a ``PeekableIterator`` (which doesn't allow a lookahead farther than 1). As a result, the parser will short-circuit and fail at the first encounter of an irrecoverable error (optional errors are processed after parsing is complete).
- The original implementation leveraged the [Shunting-yard algorithm](https://en.wikipedia.org/wiki/Shunting-yard_algorithm) (see implementation: https://github.com/oppia/oppia-android/blob/33d62aaa415bcab92dbecd9d64eda199f2676532/utility/src/main/java/org/oppia/android/util/math/MathExpressionParser.kt#L59). This implementation was far smaller, but it was much harder to make specific complex cases work correctly (such as implicit multiplication, and specialized error detection). A recursive descent parser was picked, instead, for better long-term maintainability and flexibility.
- An attempt was made to leverage a Kotlin DSL to define a generic grammar language to try and more declaratively define the implementation (i.e. by sort of just writing out the grammar and inferring a parser from it), but it ended up being more complex than the direct implementation and the syntax wasn't much better. See: 0ddcd2e.
- Specific effort was made toward maintainability in the implementation by utilizing exhaustive when statements so that new token types trigger many compiler errors (to ensure most cases that handle tokens can correctly manage the new token). This is intentional.

Some details about testing (including testing exemptions):
- Due to the size and complexity of the parser, there are actually four test suites for it: MathExpressionParserTest (verifies that parsing works at a high-level, and thoroughly tests error cases), NumericExpressionParserTest (verifies thoroughly that numeric expressions are parsed correctly), AlgebraicExpressionParserTest (thoroughly verifies variable-based parsing), and AlgebraicEquationTest (verifies equation parsing). This approach helps reduce the size of the overall suites and makes them easier to work with (Android Studio seems to have issues with Kotlin files much larger than 1k LOC).
- Note that the tests are intentionally non-redundant; they leak the implementation detail that a common implementation is used for all three types of math constructs and rely on each other for verification (i.e. AlgebraicExpressionTest assumes that most of the tests in NumericExpressionTest also apply to it, just with a variable added).
- Note that error testing does not include ordering. While it does matter to an extent, it didn't seem important enough to add targeted tests for it right now.
- I used code coverage checking locally (as part of verifying the possibility of #1497) to add a few more tests for the parser, but all the remaining uncovered lines are either impossible to cover (due to cascading when statements where earlier calls already cover such cases; this is a limitation in Kotlin) or are incorrectly instrumented by JaCoCo. The current code coverage for the parser is around 93.5% (but realistically is closer to 100%; though there are many logical cases not covered in current tests).
- MathParsingError was exempted from tests since it's a simple data structure definition.
- A custom Truth subject was added for MathParsingError.
- MathParsingErrorSubject was exempted to keep this PR smaller, and because it's trivially verifiable. #4132 was filed to track adding tests in the future.

A regex exemption check was added to enable parameterized testing in MathExpressionParserTest. These are tests which are verifying the same failure behaviors under many different conditions, so it seems like a good utilization of parameterization.

## 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
The parser introduced in this PR is not yet available via any UI features, so it doesn't directly affect UIs yet.

Commit history:

* Copy proto-based changes from #2173.

* Introduce math.proto & refactor math extensions.

Much of this is copied from #2173.

* Migrate tests & remove unneeded prefix.

* Add needed newline.

* Some needed Fraction changes.

* Introduce math expression + equation protos.

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

Most of this is copied from #2173.

* Add protos + testing lib for commutative exprs.

* Add protos & test libs for polynomials.

* Lint fix.

* Lint fixes.

* Add math tokenizer + utility & tests.

This is mostly copied from #2173.

* Add math expression/equation parsing support.

This includes full error detection, and specific test suites for each
parsing case. Much revisement is needed in the tests, and some
additional issues may yet need to be fixed in the parser and/or
error-detection logic.

This is copied from #2173 with revisement & reduction since it's part of
a multi-PR split.

* Remove unneeded comment lines.

* Fix broken test post-refactor.

* Fix equals errors for equations.

This splits the current error into two: one for no equals being present
(& adds it), and one for too many equals. This better supports the UI
errors that need to be displayed to the user in these cases.

* Post-merge fix.

* Add regex check, docs, and resolve TODOs.

This also changes regex handling in the check to be more generic for
better flexibility when matching files.

* Lint fix.

* Fix failing static checks.

* Fix broken CI checks.

Adds missing KDocs, test file exemptions, and fixes the Gradle build.

* Lint fixes.

* Add docs & exempted tests.

* Remove blank line.

* Add docs + tests.

* Add parameterized test runner.

This commit introduces a new parameterized test runner that allows
proper combinations of parameterized & non-parameterized tests in the
same suite, and in a way that should work on both Robolectric & Espresso
(though the latter isn't currently verified).

Further, this commit also introduces a TokenSubject that will be used
more explicitly by the follow-up commit for verifying MathTokenizer.

* Add & update tests.

This introduces tests for PeekableIterator, and reimplements all of
MathTokenizer's tests to be more structured, thorough, and a bit more
maintainable (i.e. by leveraging parameterized tests).

* Lint fixes.

This includes a fix for 'fun interface' not working with ktlint (see #4122).

* Remove internals that broke things.

* Add regex exemptions.

* Post-merge fix.

* Add error tests.

These tests are more or less comprehensive based on existing tests and
some new ideas. All errors are now covered by MathExpressionParserTest.

Error ordering is not tested.

A new Truth subject was added for easier testing, as well (for
MathParsingError).

* Finish algebraic equation tests.

* Reimplement numeric expression tests.

This is almost a full replacement. The new tests are more structured and
intentional to cover key high-level concepts. More tests may be added in
the future, but this is a sensible initial test offering.

This also updates MathExpressionSubject to support checking specifically
for implicit multiplication (and it's now required for such cases since
explicit is otherwise assumed).

* Finish algebraic expression tests.

These largely rely on numeric expression tests (since they focus on
verifying specific variable scenarios).

* Add missing tests for better coverage.

* Add KDocs & test exemptions.

* Lint fixes.

* Remove temporary TODOs.

* Address reviewer comments + other stuff.

This also fixes a typo and incorrectly ordered exemptions list I noticed
during development of downstream PRs.

* Move StringExtensions & fraction parsing.

This splits fraction parsing between UI & utility components.

* Address reviewer comments.

* Alphabetize test exemptions.

* Fix typo & add regex check.

The new regex check makes it so that all parameterized testing can be
more easily tracked by the Android TL.

* Add missing KDocs.

* Post-merge cleanups.

Also, fix text file exemption ordering.

* Add new test for negation with math symbol.

* Remove the ComparableOperationList wrapper.

* Change parameterized method delimiter.

* Use more intentional epsilons for float comparing.

* 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.

* Address reviewer comment.

Clarifies the documentation in the test runner around parameter
injection.

* Fix broken build.

* Fix broken build post-merge.

* Post-merge fix.

* More post-merge fixes.

* Fix TODO comment.
@Broppia Broppia added Impact: Low Low perceived user impact (e.g. edge cases). user_team and removed user_team labels Jul 29, 2022
@BenHenning BenHenning added Issue: Needs Clarification Indicates that an issue needs more detail in order to be able to be acted upon. Z-ibt Temporary label for Ben to keep track of issues he's triaged. labels Sep 16, 2022
@seanlip seanlip added enhancement End user-perceivable enhancements. and removed issue_type_infrastructure labels Mar 28, 2023
@MohitGupta121 MohitGupta121 added the Work: Medium The means to find the solution is clear, but it isn't at good-first-issue level yet. label Jun 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
enhancement End user-perceivable enhancements. Impact: Low Low perceived user impact (e.g. edge cases). Issue: Needs Clarification Indicates that an issue needs more detail in order to be able to be acted upon. Work: Medium The means to find the solution is clear, but it isn't at good-first-issue level yet. Z-ibt Temporary label for Ben to keep track of issues he's triaged.
Projects
[Team] Developer Workflow & Infrastructure - Android
Status: Todo
Milestone
No milestone
Development

No branches or pull requests
4 participants
@seanlip @BenHenning @MohitGupta121 @Broppia