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

Fix #1887: Thoroughly test FractionInputHasNumeratorEqualToRuleClassifierProvider #1927

Merged

Conversation

TheRealJessicaLi
Copy link
Contributor

@BenHenning PTAL

Explanation

Fixes #1887 by adding unit tests for FractionInputHasNumeratorEqualToRuleClassifierProvider.

Checklist

  • The PR title starts with "Fix #bugnum: ", followed by a short, clear summary of the changes. (If this PR fixes part of an issue, prefix the title with "Fix part of #bugnum: ...".)
  • The PR explanation includes the words "Fixes #bugnum: ..." (or "Fixes part of #bugnum" if the PR only partially fixes an issue).
  • The PR follows the style guide.
  • The PR does not contain any unnecessary auto-generated code from Android Studio.
  • The PR is made from a branch that's not called "develop".
  • The PR is made from a branch that is up-to-date with "develop".
  • The PR's branch is based on "develop" and not on any other branch.
  • The PR is assigned to an appropriate reviewer in both the Assignees and the Reviewers sections.

@TheRealJessicaLi TheRealJessicaLi changed the title added tests to test numerator equality Fix #1887: Thoroughly test FractionInputHasNumeratorEqualToRuleClassifierProvider Oct 3, 2020
Copy link
Member

@BenHenning BenHenning left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @TheRealJessicaLi! Apologies for the delayed review. This looks good--just left a couple of comments including some ideas for additional tests.

@TheRealJessicaLi TheRealJessicaLi force-pushed the fraction-input-numerator-equal-test branch from 220d618 to b68f3c0 Compare October 14, 2020 01:16
@TheRealJessicaLi TheRealJessicaLi force-pushed the fraction-input-numerator-equal-test branch from b68f3c0 to 4ba842b Compare October 14, 2020 01:17
@rt4914
Copy link
Contributor

rt4914 commented Oct 14, 2020

@TheRealJessicaLi Please do not do any force push. (cc @BenHenning )

Copy link
Member

@BenHenning BenHenning left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @TheRealJessicaLi. PR LGTM!

Couple things:

  1. When resolving a comment thread, please 'resolve' it by leaving a comment (not by clicking the 'resolve' button). Authors will resolve the thread after verifying it's resolved. This helps us keep better track of comments since longer PRs have dozens of them across multiple review passes.

  2. As @rt4914 mentioned, please never force submit. There are rare occasions where it's necessary, and if you hit such a case please follow up with us first. Force submission breaks history (per its nature), which leads to losing comments & conversation threads. In this case, I needed to re-review the whole PR rather than just your latest changes.

@BenHenning BenHenning merged commit b5e9c8f into oppia:develop Oct 15, 2020
Austinepass pushed a commit to Austinepass/oppia-android that referenced this pull request Oct 15, 2020
…lassifierProvider (oppia#1927)

* added tests to test numerator equality

* linted

* fixed as per comments
Austinepass pushed a commit to Austinepass/oppia-android that referenced this pull request Oct 15, 2020
…lassifierProvider (oppia#1927)

* added tests to test numerator equality

* linted

* fixed as per comments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Thoroughly test FractionInputHasNumeratorEqualToRuleClassifier
3 participants