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

Add tests for math subjects: FractionSubject, RealSubject, MathExpressionSubject, MathEquationSubject #4097

Open
BenHenning opened this issue Jan 14, 2022 · 0 comments · May be fixed by #5627
Assignees
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

#4047 introduces four new custom Truth test subjects: FractionSubject, RealSubject, MathExpressionSubject, MathEquationSubject. Each of these would be advantageous to add thorough tests for to ensure that:

  • Each of the subject & comparator methods correctly allow users to test specific properties of the protos each subject helps verify
  • Each of the subject & comparator methods are verified to fail when expected (i.e. when used with an incompatible proto property) with the correct failure reason being verified
BenHenning added a commit that referenced this issue Mar 26, 2022
…s/equations (#4047)

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

This PR introduces several new math protos:
- Real
- MathExpression (& its constituent pieces: MathBinaryOperation, MathUnaryOperation, and MathFunctionCall)
- MathEquation

The expression & equation structures are necessary for downstream numeric expression, algebraic expression, and algebraic equation parsing, as well as for implementing the MatchesExactlyWith classifiers for each interaction. For specific details on this classifier, see [the PRD](https://docs.google.com/document/d/1x2vcSjocJUXkwwlce5Gjjq_Z83ykVIn2Fp0BcnrOrTg/edit#heading=h.9mdskev1k1ax). None of that code is introduced in this PR, but will be in future PRs.

The ``Real`` structure being introduced is to help with full and partial expression evaluation that will be introduced in a future PR to ensure infinite precision can be kept for as long as possible (i.e. by keeping values in the form of integers or fractions).

Some caveats regarding the expression structure:
- Parsing information is included for token & subexpression extraction which is needed for certain user errors that will be implemented as part of the interaction UI implementations in a future PR
- A single generic structure is used to represent both numeric and algebraic expressions to simplify a lot of code (this will be more evident in future PRs)

Beyond the protos, new Truth test subject classes are being introduced to make verifying these values easier. Part of this includes a custom Kotlin DSL for verifying expressions & equations in a clean way, e.g.:

```kotlin
    val equation = parseAlgebraicEquationSuccessfully("y = (x+1)(x-1)")
    assertThat(equation).hasLeftHandSideThat().hasStructureThatMatches {
      variable {
        withNameThat().isEqualTo("y")
      }
    }
    assertThat(equation).hasRightHandSideThat().hasStructureThatMatches {
      multiplication {
        leftOperand {
          group {
            addition {
              leftOperand {
                variable {
                  withNameThat().isEqualTo("x")
                }
              }
              rightOperand {
                constant {
                  withValueThat().isIntegerThat().isEqualTo(1)
                }
              }
            }
          }
        }
        rightOperand {
          group {
            subtraction {
              leftOperand {
                variable {
                  withNameThat().isEqualTo("x")
                }
              }
              rightOperand {
                constant {
                  withValueThat().isIntegerThat().isEqualTo(1)
                }
              }
            }
          }
        }
      }
    }
```

The main benefit to the DSL is to avoid cascading checks using subsequent assertion lines that can become quite difficult to read, e.g.:

```kotlin
    val rootExpression = result.getExpectedSuccessfulExpression()
    val mulOp = rootExpression.getExpectedBinaryOperationWithOperator(MULTIPLY)
    val leftConstant = mulOp.leftOperand.getExpectedRationalConstant()
    val rightAddOp = mulOp.rightOperand.getExpectedBinaryOperationWithOperator(ADD)
    val rightAddLeftVar = rightAddOp.leftOperand.getExpectedVariable()
    val rightAddRightConstant = rightAddOp.rightOperand.getExpectedRationalConstant()
    assertThat(leftConstant).isEqualTo(createWholeNumberFraction(2))
    assertThat(rightAddLeftVar).isEqualTo("x")
    assertThat(rightAddRightConstant).isEqualTo(createWholeNumberFraction(1))
```

The above is for the expression "2(x+1)" and was a previous attempt. It leverages helpers and to me seems much harder to read as compared with the DSL. The DSL does result in more verbosity, but I think that actually helps readability in this case.

### Test exemptions
Note that I did not add tests for these. Given that they're test utilities & the importance of getting the downstream work completed, I decided to file #4097 to track adding tests for each. I've already manually verified the subjects work in actual tests downstream, and the code is simple enough that I'm confident in its correctness by inspection. Nevertheless, we should eventually add automated tests for this. Reviewers: please let me know if you have any concerns with this approach.

## 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
N/A -- proto & testing library only change, and the proto changes are only additions. No UI functionality is yet affected by these changes.

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.

* Lint fix.

* Fix broken test post-refactor.

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

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

* Add missing KDocs.

* Fix broken build.

* Fix broken build post-merge.

* Post-merge fix.

* More post-merge fixes.
@Broppia Broppia added Impact: Low Low perceived user impact (e.g. edge cases). 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
@manas-yu manas-yu self-assigned this Dec 27, 2024
@manas-yu manas-yu linked a pull request Dec 28, 2024 that will close this issue
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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.
Development

Successfully merging a pull request may close this issue.

5 participants