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 math subjects: FractionSubject, RealSubject, MathExpressionSubject, MathEquationSubject #4097

Closed
BenHenning opened this issue Jan 14, 2022 · 3 comments · Fixed by #5627 or #5655
Closed

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

BenHenning opened this issue Jan 14, 2022 · 3 comments · Fixed by #5627 or #5655
Assignees
@manas-yu
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
This was referenced Jan 14, 2022
Merged
Open
Closed
This was referenced Jan 21, 2022
Closed
Closed
@BenHenning BenHenning mentioned this issue Mar 8, 2022
Open
BenHenning added a commit that referenced this issue Mar 26, 2022
@BenHenning
Fix part of #4044: Add protos & testing libraries for math expression…
0b1e018 
…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.
@BenHenning BenHenning mentioned this issue Mar 28, 2022
Open
@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 mentioned this issue Dec 28, 2024
Merged
6 tasks
@github-actions github-actions bot reopened this Jan 15, 2025
@github-actions GitHub Actions
Copy link

The issue is reopened because of the following unresolved TODOs:

oppia-android/testing/src/main/java/org/oppia/android/testing/math/FractionSubject.kt

Line 13 in fc92932

// TODO(#4097): Add tests for this class.

oppia-android/testing/src/main/java/org/oppia/android/testing/math/MathEquationSubject.kt

Line 13 in fc92932

// TODO(#4097): Add tests for this class.

oppia-android/testing/src/main/java/org/oppia/android/testing/math/MathExpressionSubject.kt

Line 26 in fc92932

// TODO(#4097): Add tests for this class.

oppia-android/testing/src/main/java/org/oppia/android/testing/math/RealSubject.kt

Line 13 in fc92932

// TODO(#4097): Add tests for this class.

@manas-yu
Copy link
Collaborator

manas-yu commented Jan 15, 2025 •
edited
Loading

Hey @adhiamboperes, I wasn't aware of the need to resolve TODOs earlier. I can create a new PR that resolves TODOs for both #4097 and #4848 in a single submission. I hope that simply removing the TODO comments will resolve these issues. I'll be sure to address this in future PRs as well.

@adhiamboperes
Copy link
Collaborator

adhiamboperes commented Jan 15, 2025 •
edited
Loading

@manas-yu, TODOs can be removed as long as the comment associated with it has been addressed. In this case, the test cases have been added, so they can safely be removed

@manas-yu manas-yu mentioned this issue Jan 17, 2025
Merged
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@manas-yu manas-yu

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
Archived in project
Milestone
No milestone
6 participants
@seanlip @BenHenning @adhiamboperes @MohitGupta121 @Broppia @manas-yu