-
Notifications
You must be signed in to change notification settings - Fork 521
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 #1898: add unit tests for FractionInputIsExactlyEqualToRuleClassifierProvider #1924
Fix #1898: add unit tests for FractionInputIsExactlyEqualToRuleClassifierProvider #1924
Conversation
Hi! @TheRealJessicaLi, Welcome to Oppia! Please could you follow the instructions here to get started? You'll need to do this before we can accept your PR. I am closing this PR for now. Feel free to re-open it once you are done with the above instructions. Thanks! |
Looks like when you filled out the form, there was an extra space before your name which caused Oppiabot to get confused @TheRealJessicaLi :). It shouldn't auto-close this time. |
@TheRealJessicaLi it looks like you have lint failures. Please look at https://github.com/oppia/oppia-android/wiki/Ktlint-Guide for how to setup & run ktlint locally. |
Fixed, thanks! |
Thought to mention here, we also need to remove the TODO comment from it's respective |
linted removed comment
74d87c8
to
06d429c
Compare
Fixed, thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @TheRealJessicaLi! This looks really good! I honestly only have a few nits about styling & naming for the tests. Coverage wise this looks great!
...main/classify/rules/fractioninput/FractionInputIsExactlyEqualToRuleClassifierProviderTest.kt
Outdated
Show resolved
Hide resolved
...main/classify/rules/fractioninput/FractionInputIsExactlyEqualToRuleClassifierProviderTest.kt
Outdated
Show resolved
Hide resolved
...main/classify/rules/fractioninput/FractionInputIsExactlyEqualToRuleClassifierProviderTest.kt
Outdated
Show resolved
Hide resolved
...main/classify/rules/fractioninput/FractionInputIsExactlyEqualToRuleClassifierProviderTest.kt
Outdated
Show resolved
Hide resolved
...main/classify/rules/fractioninput/FractionInputIsExactlyEqualToRuleClassifierProviderTest.kt
Outdated
Show resolved
Hide resolved
} | ||
|
||
// TODO(#89): Move to a common test library. | ||
private fun <T : Throwable> assertThrows(type: KClass<T>, operation: () -> Unit): T { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rt4914 solving this might be a nice SLoP project idea if we split it up & update the TODOs (or just require that students search for them).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one too TODO(#59): Figure out a way to reuse modules instead of needing to re-declare them.
But I think it will be tricky.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@MohamedMedhat1998 I don't think that's actually possible without something like Dagger Hilt which, as far as I'm aware, doesn't yet have full Gradle support. I was planning to adopt that once Bazel work is completed.
Thanks for the tips, just refactored everything to name things more explicitly |
fixed lint issues
c2579e7
to
a7a254e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is excellent, thanks @TheRealJessicaLi! I have no further comments. :) Going ahead and merging this.
@rt4914 PTAL
Explanation
Fixed #1898 by adding testing for
FractionInputIsExactlyEqualToRuleClassifierProvider
which was copied from the Oppia web repo.Checklist