-
Notifications
You must be signed in to change notification settings - Fork 528
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
Fixes #4135 : Added FractionInputInteractionViewTestActivityTest #4268
Conversation
@bhaktideshmukh Please assign me back once the GitHub actions checks are passed completely. |
Hi @bhaktideshmukh, I'm going to mark this PR as stale because it hasn't had any updates for 7 days. If no further activity occurs within 7 days, it will be automatically closed so that others can take up the issue. |
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.
Took a higher-level view and left some comments.
Assign me back once the suggested items and GitHub actions get fixed.
app/build.gradle
Outdated
@@ -7,7 +7,7 @@ apply plugin: 'kotlin-kapt' | |||
|
|||
android { | |||
compileSdkVersion 30 | |||
buildToolsVersion "29.0.2" | |||
buildToolsVersion '29.0.2' |
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.
Why do we need to update this?
@@ -20,7 +20,7 @@ import org.oppia.android.util.math.FractionParser | |||
import javax.inject.Inject | |||
|
|||
/** [StateItemViewModel] for the fraction input interaction. */ | |||
class FractionInteractionViewModel private constructor( | |||
class FractionInteractionViewModel public constructor( |
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.
why do we need to modify this?
By default, all constructors are public , which effectively amounts to them being visible everywhere the class is visible
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.
Do you mean that we need to remove the access modifier since all constructors are public by default?
@@ -124,16 +123,13 @@ class InputInteractionViewTestActivity : | |||
.create(interaction = params.interaction) | |||
} | |||
} | |||
|
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.
Why we need to remove this line?
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.
Also, we don't need line 58 and 59, please check if we need them here.
import javax.inject.Inject | ||
import javax.inject.Singleton | ||
|
||
/** Tests for [FractionInputInteractionViewTestActivity] */ |
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.
/** Tests for [FractionInputInteractionViewTestActivity] */ | |
/** Tests for [FractionInputInteractionViewTestActivity]. */ |
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.
Done
Please address all the comments above and assign me back once done and the github actions failure gets fixed. |
…oid into fraction_input
@BenHenning The I am unable to understand the reason behind this. |
MathExpressionInteractionsViewTest is failing, though it doesn't seem like it was changed in this PR. Oddly, we haven't seen many other PRs hit this but given the sudden lull in PR activity it's possible we just haven't had enough runs with that the final math expression PR (#2173) that introduced the test, and the test, to trigger the issue. I'll check locally to see if it's an actual flake. |
I'm also re-running the failed workflows to see if they repro the issue. I'm a bit suspicious that this is actually a flake since both Gradle & Bazel versions of the test failed, and I haven't seen this failure before. |
What should be done next? |
@bhaktideshmukh as far as I can tell per other ongoing PRs the test failures seem new in this PR. I think you'll need to investigate this. I suggest:
|
I am bit busy till 14th may, will resume the work on this PR from 15th of may. |
Hi @bhaktideshmukh, I'm going to mark this PR as stale because it hasn't had any updates for 7 days. If no further activity occurs within 7 days, it will be automatically closed so that others can take up the issue. |
Hi. Since I was requested as a reviewer on this PR: Please note that I will be away & unavailable for reviews starting today until 23 May 2022. Please try to make any progress that you can which doesn't require me, and I'll catch up on my reviews when I return. Thanks for your flexibility--I really appreciate it. |
Hi @bhaktideshmukh, I'm going to mark this PR as stale because it hasn't had any updates for 7 days. If no further activity occurs within 7 days, it will be automatically closed so that others can take up the issue. |
Explanation
Fixes #4135 : Added FractionInputInteractionViewTestActivityTest
Seperated fraction tests from InputInteractionViewTestActivityTest to new specific file for fraction tests.
Essential Checklist
For UI-specific PRs only
If your PR includes UI-related changes, then: