Skip to content

Commit

Permalink
Fix part of #4044: Add protos & testing libraries for math expression…
Browse files Browse the repository at this point in the history
…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.
  • Loading branch information
BenHenning authored Mar 26, 2022
1 parent 240bdb6 commit 0b1e018
Show file tree
Hide file tree
Showing 9 changed files with 937 additions and 7 deletions.
183 changes: 176 additions & 7 deletions model/src/main/proto/math.proto
Original file line number Diff line number Diff line change
Expand Up @@ -5,17 +5,186 @@ package model;
option java_package = "org.oppia.android.app.model";
option java_multiple_files = true;

// Structure for a fraction object.
// Represents a fraction.
//
// Values of this proto can be analyzed using FractionSubject.
message Fraction {
// Defines whether the fraction is negative.
bool is_negative = 1;
int32 whole_number = 2;
int32 numerator = 3;
int32 denominator = 4;

// Defines the whole number component of the fraction.
uint32 whole_number = 2;

// Defines the numerator of the fraction.
uint32 numerator = 3;

// Defines the denominator of the fraction. This should never be zero.
uint32 denominator = 4;
}

// Structure containing a ratio object for eg - [1,2,3] for 1:2:3.
// Represents a structured real value.
//
// Values of this proto can be analyzed using RealSubject.
message Real {
// Defines type of real value.
oneof real_type {
// Indicates that this real value is a fraction.
Fraction rational = 1;

// Indicates that this real value is a decimal. Technically these can sometimes be rational, but
// given IEEE-754 rounding errors and the difficulty of factoring fractions, many rational
// decimal values need to be treated as irrational and non-factorable.
double irrational = 2;

// Indicates that this real value is an integer (as a special case of rational values since
// integers are easier to work with than fraction objects). Note that this isn't the only case
// where the real value can be an integer. It can also be an integer double value, or a fraction
// with only a whole number component.
int32 integer = 3;
}
}

// Represents a ratio, e.g. 1:2:3.
message RatioExpression {
// List of components in a ratio. It's expected that list should have more than
// 1 element.
// List of components in a ratio. For example, the ratio 1:2:3 will have component values
// [1, 2, 3]. It's expected that list should have more than 1 element.
repeated uint32 ratio_component = 1;
}

// Represents a mathematical expression such as 1+2*7. Expressions are inherently recursive, so the
// overall expressiveness of this structure (& math expressions as a whole) is defined based on its
// constituent substructures.
//
// This structure is designed to represent both numeric and algebraic expressions.
//
// Values of this proto can be analyzed using MathExpressionSubject.
message MathExpression {
// The index within the input text stream at which point the expression starts (it's an inclusive
// index). If both this and the end index are the same then no parsing information is included for
// this specific expression.
uint32 parse_start_index = 1;

// The index within the input text stream at which point the expression ends, exclusively. If both
// this and the start index are the same then no parsing information is included for this specific
// expression.
uint32 parse_end_index = 2;

// The type of expression.
oneof expression_type {
// Indicates that this expression is a real value.
Real constant = 3;

// Indicates that this expression is a variable (which is not valid for numeric-only
// expressions).
string variable = 4;

// Indicates that this expression is a binary operation between two sub-expressions.
MathBinaryOperation binary_operation = 5;

// Indicates that this expression is a unary operation that's operating on a sub-expression.
MathUnaryOperation unary_operation = 6;

// Indicates that this expression is a function call with a sub-expression argument.
MathFunctionCall function_call = 7;

// Indicates that this expression represents a nested group, e.g. 1+(2+3).
MathExpression group = 8;
}
}

// Represents a binary operation like addition or multiplication.
//
// Values of this proto can be analyzed using MathExpressionSubject (within the context of a
// MathExpression).
message MathBinaryOperation {
// Types of supported binary operations.
enum Operator {
// Represents an unknown operator (which is never supported).
OPERATOR_UNSPECIFIED = 0;

// Represents adding two values, e.g.: 1+x.
ADD = 1;

// Represents subtracting two values, e.g.: x-2.
SUBTRACT = 2;

// Represents multiplying two values, e.g.: x*y.
MULTIPLY = 3;

// Represents dividing two values, e.g.: 1/x.
DIVIDE = 4;

// Represents taking the exponentiation of one value by another, e.g.: x^2.
EXPONENTIATE = 5;
}

// The type of binary operation.
Operator operator = 1;

// The left-hand side of the operation, e.g. the '1' in 1+2.
MathExpression left_operand = 2;

// The right-hand side of the operation, e.g. the '2' in 1+2.
MathExpression right_operand = 3;

// Indicates whether this operation is implicit. This is currently only supported for
// multiplication, and helps represent expressions like '2x' (which should be treated as 2*x).
bool is_implicit = 4;
}

// Represents a unary operation like negation.
//
// Values of this proto can be analyzed using MathExpressionSubject (within the context of a
// MathExpression).
message MathUnaryOperation {
// Types of supported unary operations.
enum Operator {
// Represents an unknown operator (which is never supported).
OPERATOR_UNSPECIFIED = 0;

// Represents negating a value, e.g.: -y.
NEGATE = 1;

// Represents indicating a value as positive, e.g.: +y.
POSITIVE = 2;
}

// The type of unary operation, e.g. the '1' in -1.
Operator operator = 1;

// The operand being operated upon.
MathExpression operand = 2;
}

// Represents a function call, like square root.
//
// Values of this proto can be analyzed using MathExpressionSubject (within the context of a
// MathExpression).
message MathFunctionCall {
// The types of supported function calls.
enum FunctionType {
// Represents an unknown function (which is never supported).
FUNCTION_UNSPECIFIED = 0;

// Represents a square root operation, e.g. sqrt(4).
SQUARE_ROOT = 1;
}

// The type of function being called within this subexpression.
FunctionType function_type = 1;

// The subexpression being passed as an argument to the function.
MathExpression argument = 2;
}

// Represents a mathematical equation (generally algebraic) such as: 2x+3y=0.
//
// Values of this proto can be analyzed using MathEquationSubject.
message MathEquation {
// The MathExpression representing the left-hand side of the equation, e.g. the '2x+3y' in
// 2x+3y=0.
MathExpression left_side = 1;

// The MathExpression representing the left-hand side of the equation, e.g. the '0' in 2x+3y=0.
MathExpression right_side = 2;
}
4 changes: 4 additions & 0 deletions scripts/assets/test_file_exemptions.textproto
Original file line number Diff line number Diff line change
Expand Up @@ -648,6 +648,10 @@ exempted_file_path: "testing/src/main/java/org/oppia/android/testing/espresso/Ge
exempted_file_path: "testing/src/main/java/org/oppia/android/testing/espresso/ImageViewMatcher.kt"
exempted_file_path: "testing/src/main/java/org/oppia/android/testing/espresso/KonfettiViewMatcher.kt"
exempted_file_path: "testing/src/main/java/org/oppia/android/testing/junit/DefineAppLanguageLocaleContext.kt"
exempted_file_path: "testing/src/main/java/org/oppia/android/testing/math/FractionSubject.kt"
exempted_file_path: "testing/src/main/java/org/oppia/android/testing/math/MathEquationSubject.kt"
exempted_file_path: "testing/src/main/java/org/oppia/android/testing/math/MathExpressionSubject.kt"
exempted_file_path: "testing/src/main/java/org/oppia/android/testing/math/RealSubject.kt"
exempted_file_path: "testing/src/main/java/org/oppia/android/testing/mockito/MockitoKotlinHelper.kt"
exempted_file_path: "testing/src/main/java/org/oppia/android/testing/network/ApiMockLoader.kt"
exempted_file_path: "testing/src/main/java/org/oppia/android/testing/network/MockClassroomService.kt"
Expand Down
1 change: 1 addition & 0 deletions testing/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ load("//testing:testing_test.bzl", "testing_test")
# globs here to ensure new files added to migrated packages don't accidentally get included in the
# top-level module library.
MIGRATED_PROD_FILES = glob([
"src/main/java/org/oppia/android/testing/math/*.kt",
"src/main/java/org/oppia/android/testing/mockito/*.kt",
"src/main/java/org/oppia/android/testing/networking/*.kt",
"src/test/java/org/oppia/android/testing/platformparameter/*.kt",
Expand Down
2 changes: 2 additions & 0 deletions testing/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -76,13 +76,15 @@ dependencies {
'com.google.dagger:dagger:2.24',
'com.google.protobuf:protobuf-javalite:3.17.3',
'com.google.truth:truth:1.1.3',
'com.google.truth.extensions:truth-liteproto-extension:1.1.3',
'nl.dionsegijn:konfetti:1.2.5',
'org.jetbrains.kotlinx:kotlinx-coroutines-test:1.2.2',
'org.robolectric:robolectric:4.5',
'org.jetbrains.kotlin:kotlin-reflect:$kotlin_version',
'org.jetbrains.kotlin:kotlin-stdlib:$kotlin_version',
'org.mockito:mockito-core:2.19.0',
project(":domain"),
project(":model"),
project(":utility"),
)
compileOnly(
Expand Down
75 changes: 75 additions & 0 deletions testing/src/main/java/org/oppia/android/testing/math/BUILD.bazel
Original file line number Diff line number Diff line change
@@ -0,0 +1,75 @@
"""
General testing utilities and truth subjects for math structures and utilities.
"""

load("@io_bazel_rules_kotlin//kotlin:kotlin.bzl", "kt_android_library")

# TODO(#2747): Move these libraries to be under utility/.../math/testing.

kt_android_library(
name = "fraction_subject",
testonly = True,
srcs = [
"FractionSubject.kt",
],
visibility = [
"//:oppia_testing_visibility",
],
deps = [
"//model/src/main/proto:math_java_proto_lite",
"//third_party:com_google_truth_extensions_truth-liteproto-extension",
"//third_party:com_google_truth_truth",
"//utility/src/main/java/org/oppia/android/util/math:extensions",
],
)

kt_android_library(
name = "math_equation_subject",
testonly = True,
srcs = [
"MathEquationSubject.kt",
],
visibility = [
"//:oppia_testing_visibility",
],
deps = [
":math_expression_subject",
"//model/src/main/proto:math_java_proto_lite",
"//third_party:com_google_truth_extensions_truth-liteproto-extension",
"//third_party:com_google_truth_truth",
],
)

kt_android_library(
name = "math_expression_subject",
testonly = True,
srcs = [
"MathExpressionSubject.kt",
],
visibility = [
"//:oppia_testing_visibility",
],
deps = [
":real_subject",
"//model/src/main/proto:math_java_proto_lite",
"//third_party:com_google_truth_extensions_truth-liteproto-extension",
"//third_party:com_google_truth_truth",
],
)

kt_android_library(
name = "real_subject",
testonly = True,
srcs = [
"RealSubject.kt",
],
visibility = [
"//:oppia_testing_visibility",
],
deps = [
":fraction_subject",
"//model/src/main/proto:math_java_proto_lite",
"//third_party:com_google_truth_extensions_truth-liteproto-extension",
"//third_party:com_google_truth_truth",
],
)
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
package org.oppia.android.testing.math

import com.google.common.truth.BooleanSubject
import com.google.common.truth.DoubleSubject
import com.google.common.truth.FailureMetadata
import com.google.common.truth.IntegerSubject
import com.google.common.truth.Truth.assertAbout
import com.google.common.truth.Truth.assertThat
import com.google.common.truth.extensions.proto.LiteProtoSubject
import org.oppia.android.app.model.Fraction
import org.oppia.android.util.math.toDouble

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

/**
* Truth subject for verifying properties of [Fraction]s.
*
* Note that this class is also a [LiteProtoSubject] so other aspects of the underlying [Fraction]
* proto can be verified through inherited methods.
*
* Call [assertThat] to create the subject.
*/
class FractionSubject private constructor(
metadata: FailureMetadata,
private val actual: Fraction
) : LiteProtoSubject(metadata, actual) {
/**
* Returns a [BooleanSubject] to test [Fraction.getIsNegative]. This method never fails since the
* underlying property defaults to false if it's not defined in the fraction.
*/
fun hasNegativePropertyThat(): BooleanSubject = assertThat(actual.isNegative)

/**
* Returns an [IntegerSubject] to test [Fraction.getWholeNumber]. This method never fails since
* the underlying property defaults to 0 if it's not defined in the fraction.
*/
fun hasWholeNumberThat(): IntegerSubject = assertThat(actual.wholeNumber)

/**
* Returns an [IntegerSubject] to test [Fraction.getNumerator]. This method never fails since the
* underlying property defaults to 0 if it's not defined in the fraction.
*/
fun hasNumeratorThat(): IntegerSubject = assertThat(actual.numerator)

/**
* Returns an [IntegerSubject] to test [Fraction.getDenominator]. This method never fails since
* the underlying property defaults to 0 if it's not defined in the fraction.
*/
fun hasDenominatorThat(): IntegerSubject = assertThat(actual.denominator)

/**
* Returns a [DoubleSubject] to test the converted double version of the fraction being
* represented by this subject.
*/
fun evaluatesToDoubleThat(): DoubleSubject = assertThat(actual.toDouble())

companion object {
/** Returns a new [FractionSubject] to verify aspects of the specified [Fraction] value. */
fun assertThat(actual: Fraction): FractionSubject = assertAbout(::FractionSubject).that(actual)
}
}
Loading

0 comments on commit 0b1e018

Please sign in to comment.