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 ComparableOperationSubject #4098

Open
BenHenning opened this issue Jan 14, 2022 · 3 comments
Open

Add tests for ComparableOperationSubject #4098

BenHenning opened this issue Jan 14, 2022 · 3 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

BenHenning commented Jan 14, 2022 •
edited
Loading

#4049 introduces a new custom Truth test subject: ComparableOperationSubject. It would be advantageous to add thorough tests for this 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

See #4097 for a similar issue.
@BenHenning BenHenning mentioned this issue Jan 14, 2022
Merged
6 tasks
@ayush0402
Copy link
Contributor

ayush0402 commented Jan 16, 2022 •
edited
Loading

@BenHenning Can I work on this once #4049 is merged?

@BenHenning
Copy link
Member Author

Sure @ayush0402, but please be aware that it may be a few weeks before that PR is merged (I plan to merge the entire sequence of ~17 PRs in one shot, so they'll all need to be finalized before any are merged).

@BenHenning BenHenning changed the title Add tests for ComparableOperationListSubject Add tests for ComparableOperationSubject Jan 28, 2022
BenHenning added a commit that referenced this issue Mar 26, 2022
@BenHenning
Fix part of #4044: Add protos & testing library for commutative expre…
4805c65 
…ssions/operations (#4049)

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

This PR introduces the proto & Truth subject for representing math expressions in a way that makes them trivial to compare without commutative or associativity changing the meaning of the operation. See #4047 for details about the test exemptions being made here, and the custom Kotlin DSL. Note that #4098 is tracking adding tests for the new test subject.

The way the structure introduced here works is that it provides a structure for math expressions that collects "accumulations" (i.e. products or summations). Since both multiplication and addition are commutative operators, elements of these accumulation lists can be rearranged (and thus sorted for deterministic sorting that doesn't fail upon commutative reordering). Associativity complicates this since we actually want expressions like 3-4+7 to be equivalent to 7+3-4. In order for that to be the case, the structure treats subtraction as negative addition and division as inverted multiplication so that division and subtraction terms can also be rearranged. This leaves only square roots & exponentiations as non-commutative operations (which are more or less retained in the same hierarchical structure as math expressions).

This structure will be utilized in a later PR when a routine for converting ``MathExpression``s to ``ComparableOperation``s, and then later in MatchesUpToTrivialManipulations 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.g0bd549g079d).

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

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

* Add docs & exempted tests.

* Remove blank line.

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

* Remove the ComparableOperationList wrapper.

* Fix broken build.

* Fix broken build post-merge.

* Post-merge fix.

* More post-merge fixes.
@BenHenning
Copy link
Member Author

Note that #4049 is now merged, so this & other subject test-related issues should now be available to work on.

@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
5 participants
@seanlip @BenHenning @ayush0402 @MohitGupta121 @Broppia