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 PolynomialSubject #4100

Closed
BenHenning opened this issue Jan 14, 2022 · 0 comments · Fixed by #5667
Closed

Add tests for PolynomialSubject #4100

BenHenning opened this issue Jan 14, 2022 · 0 comments · Fixed by #5667
Assignees
@theayushyadav11
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

BenHenning commented Jan 14, 2022 •
edited
Loading

#4050 introduces a new custom Truth test subject: PolynomialSubject. 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 of the protos each subject helps verify
  • Each of the subject methods are verified to fail when expected (i.e. when used with an incompatible proto property) with the correct failure reason being verified

See #4097 for a similar issue.
@BenHenning BenHenning mentioned this issue Jan 14, 2022
Merged
6 tasks
BenHenning added a commit that referenced this issue Mar 26, 2022
@BenHenning
Fix part of #4044: Add protos & testing library for polynomials (#4050)
7e75830 
## Explanation
Fix part of #4044
Originally copied from #2173 when it was in proof-of-concept form

This PR introduces the protos and test subject to represent polynomials.

For the purposes of upcoming classifier work, a polynomial is defined as a sum of terms where each term has a coefficient and zero or more variables with positive integer powers. This representation provides support for all real polynomials, and keeps a nicely structured representation for coefficients that actually allows for retaining integers and rational values (this will become more evident when math expression -> polynomial conversion is added).

Furthermore, various extensions files are added to help support some of the fundamental operations (mainly needed by test subjects). These operations are being thoroughly tested, and will be augmented with a lot more functionality in upcoming PRs.

The new polynomial test subject doesn't have new tests to keep this PR focused on production code, and since it's relatively easy to verify as correct via review. #4100 is tracking adding tests.

This structure will be utilized in a later PR in IsEquivalentTo classifier implementations for each numeric expression, algebraic expression, and math equation interaction. For specific details on this classifier, see [the PRD](https://docs.google.com/document/d/1x2vcSjocJUXkwwlce5Gjjq_Z83ykVIn2Fp0BcnrOrTg/edit#heading=h.1q3av9yssyi5). Polynomials are the ideal structure for verifying equivalence since they fully collapse expressions regardless of associativity, commutativity, and distributivity (to some extent--caveats will be noted in the future PR that converts expressions to this new polynomial structure).

Slightly separate from polynomials, ``FloatExtensions`` was updated to include better epsilon values for comparing both floats and doubles (and a separate one is used for doubles). These are loosely based on the computed machine epsilon value listed here: https://en.wikipedia.org/wiki/Machine_epsilon, but it uses a higher order of magnitude and rounding for a bit more "wiggle room" for equality (so that it's not essentially replicating '==' for values close to 1).

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

* Add protos + testing lib for commutative exprs.

* Add protos & test libs for polynomials.

* Lint fix.

* Lint fixes.

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

* Add docs & exempted tests.

* Remove blank line.

* Add docs + tests.

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

* Add missing KDocs.

* Remove the ComparableOperationList wrapper.

* Use more intentional epsilons for float comparing.

* Remove failing test.

* 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 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
@theayushyadav11 theayushyadav11 self-assigned this Jan 23, 2025
@theayushyadav11 theayushyadav11 mentioned this issue Jan 26, 2025
Merged
6 tasks
adhiamboperes pushed a commit that referenced this issue Feb 11, 2025
@theayushyadav11
Fixes #4100 : Added tests for PolynomialSubject (#5667)
c2c81bd 
Fix #4100
### New Tests for Polynomial Functionality:

* **Build Configuration Update:**
* Added a new test target `PolynomialSubjectTest` to the `BUILD.bazel`
file. This includes necessary dependencies and configurations for the
new test.

* **New Test File:**
* Created `PolynomialSubjectTest.kt` which contains various test cases
to validate polynomial operations. This includes tests for checking
invalid polynomials, constant polynomials, term counts, and polynomial
evaluations.

#### Screenshot of the passing tests.
![Screenshot from 2025-01-26
16-03-01](https://github.com/user-attachments/assets/e194d152-e4fc-4840-a30d-fa60f739ff81)



## Essential Checklist
<!-- Please tick the relevant boxes by putting an "x" in them. -->
- [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)).
This pull request introduces a new set of tests for polynomial-related
functionality in the `oppia_android_test` project. The primary changes
include the addition of a new test file and corresponding updates to the
build configuration.

---------

Co-authored-by: theayushyadav11 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@theayushyadav11 theayushyadav11

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: Done
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fixes #4100 : Added tests for PolynomialSubject theayushyadav11/oppia-android
5 participants
@seanlip @BenHenning @MohitGupta121 @Broppia @theayushyadav11