Fix #1889 : Throughly test `NumericInputIsWithinToleranceRuleClassifier` #1922

prayutsu · 2020-09-30T07:38:50Z

Explanation

Fixes #1889 : Test cases are added to thoroughly test NumericInputIsWithinToleranceRuleClassifier

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.

All tests are passing -

prayutsu · 2020-09-30T07:41:30Z

@MohamedMedhat1998 PTAL

...domain/classify/rules/numberinput/NumericInputIsWithinToleranceRuleClassifierProviderTest.kt

aggarwalpulkit596 · 2020-09-30T07:57:29Z

...domain/classify/rules/numberinput/NumericInputIsWithinToleranceRuleClassifierProviderTest.kt

+  }
+
+  @Test
+  fun testIntAnswer_secondStringInput_throwsException() {


this one seems a bit redundant to me as we have already checked that the second input won't work with string. Fix this elsewhere too

prayutsu · 2020-09-30T09:05:09Z

@aggarwalpulkit596 I have also updated the screenshot in the PR description. Please take a look.

...domain/classify/rules/numberinput/NumericInputIsWithinToleranceRuleClassifierProviderTest.kt

aggarwalpulkit596 · 2020-09-30T09:12:37Z

...domain/classify/rules/numberinput/NumericInputIsWithinToleranceRuleClassifierProviderTest.kt

+  }
+
+  @Test
+  fun testIntAnswer_missingSecondInput_throwsException() {


same as above

aggarwalpulkit596 · 2020-09-30T09:18:49Z

...domain/classify/rules/numberinput/NumericInputIsWithinToleranceRuleClassifierProviderTest.kt

+  }
+
+  @Test
+  fun testRealAnswer_firstStringInput_throwsException() {


Add a test case with 3 inputs as well and I guess i wasn't clear enough in my previous comments but what i wanted was to have kept the existing test cases in which there was incorrect input variable like c or anything

If I test with 3 inputs, exception's error is a bit weird

@aggarwalpulkit596 Also isn't that case covered in missing inputs and incorrect inputs ?

@prayutsu what's the error for 3 inputs?

@MohamedMedhat1998 What do you think?

@prayutsu having test cases that check the case where we have 3 inputs is a good idea.
I suggest trying to pass a map that contains both "x" and "tol" + some other value ("y" for example) and observe which exception you will get. After you know the exception, you can write a test case that checks whether this exception is thrown or not.
It's kinda weird to gerenrate an exception then test whether it is generated or not 😅, but I think it is ok.

@MohamedMedhat1998 I tried the same, but see the exception in the screenshot, it does not throw the exception of the "same type" that we get in other test cases

@aggarwalpulkit596 I think this will not work because, currently exception message is of the form Expected classifier inputs to contain parameter with name 'x' but had: [c, tol] but in case of three inputs it can't display the range

Please, see this comment @MohamedMedhat1998

@MohamedMedhat1998 I tried the same, but see the exception in the screenshot, it does not throw the exception of the "same type" that we get in other test cases

Can you share your code, please?

MohamedMedhat1998

The overall PR is very good but I have left some comments, PTAL.

MohamedMedhat1998 · 2020-09-30T11:36:52Z

...domain/classify/rules/numberinput/NumericInputIsWithinToleranceRuleClassifierProviderTest.kt

+  }
+
+  @Test
+  fun testPositiveIntAnswer_positiveRealInput_positiveRealTolerance_answerWithinTolerance() {


testPositiveIntAnswer_positiveRealInput_positiveRealTolerance_answerWithinTolerance to testPositiveRealAnswer_positiveRealInput_positiveRealTolerance_answerWithinTolerance

@MohamedMedhat1998 Test cases with Int are covering separate tests, they have been added intentionally, please confirm once and I'll change it.

I see that you have added test cases for both int and real, but in this test case you are using real as an answer (POSITIVE_REAL_VALUE_2_5) but the name is int

Okay this would be fixed in the next commit

...domain/classify/rules/numberinput/NumericInputIsWithinToleranceRuleClassifierProviderTest.kt

MohamedMedhat1998 · 2020-09-30T11:57:06Z

...domain/classify/rules/numberinput/NumericInputIsWithinToleranceRuleClassifierProviderTest.kt

+  }
+
+  @Test
+  fun testNegativeRealAnswer_negativeRealInput_positiveIntTolerance_answerWithinTolerance() {


testNegativeRealAnswer_negativeRealInput_positiveIntTolerance_answerWithinTolerance to testNegativeIntAnswer_negativeIntInput_positiveIntTolerance_answerWithinTolerance

Was this fixed?

yup, this is fixed now

...domain/classify/rules/numberinput/NumericInputIsWithinToleranceRuleClassifierProviderTest.kt

prayutsu · 2020-09-30T15:34:01Z

@MohamedMedhat1998 Note - All changes are not fixed yet

prayutsu · 2020-09-30T15:35:06Z

...domain/classify/rules/numberinput/NumericInputIsWithinToleranceRuleClassifierProviderTest.kt

+
+  @Test
+  fun testIntAnswer_extraInput_throwsException() {
+    val inputs = mapOf(


@MohamedMedhat1998 This is the testcase

Ok, thanks.
Btw you didn't have to push the changes. If you are unsure of the code and you don't want to push it, you can simply add it as a code snippet in a comment like this:

@Test fun testIntAnswer_extraInput_throwsException() { val inputs = mapOf( "c" to POSITIVE_INT_VALUE_2, "x" to POSITIVE_INT_VALUE_3, "tol" to POSITIVE_INT_VALUE_1 ) val exception = assertThrows(IllegalStateException::class) { inputIsWithinToleranceRuleClassifier .matches(answer = POSITIVE_INT_VALUE_2, inputs = inputs) } assertThat(exception) .hasMessageThat() .contains("Expected classifier inputs to contain parameter with name 'tol' but had: [x]") }

Try removing any assertion related to the exception and simply use an incorrect map in the normal test cases like testNegativeRealAnswer_negativeRealInput_positiveRealTolerance_answerWithinTolerance and observe the output

okay, thanks

I tried this -

@Test fun testIntAnswer_extraInput_throwsException() { val inputs = mapOf( "c" to POSITIVE_INT_VALUE_2, "x" to POSITIVE_INT_VALUE_3, "tol" to POSITIVE_INT_VALUE_1 ) val matches = inputIsWithinToleranceRuleClassifier .matches(answer = POSITIVE_INT_VALUE_2, inputs = inputs) assertThat(matches).isFalse() }

And the result is -

private fun <T : Throwable> assertThrows(type: KClass<T>, operation: () -> Unit): T { try { operation() fail("Expected to encounter exception of $type") } catch (t: Throwable) { if (type.isInstance(t)) { return type.cast(t) } // Unexpected exception; throw it. throw t } }

@MohamedMedhat1998 I think we are getting an unexpected exception

This is sort of a general "how do we handle exceptional cases" question. In this case, adding a check for extra input helps make things more predictable, but extra parameters don't actually interfere with the classifier's ability to do its job.

I'm also curious to know what others think about this.

@MohamedMedhat1998 I think we can add test to check exceptions.

@MohamedMedhat1998 I think we can add test to check exceptions.

But actually, no exception is thrown in this case. So, whenever we add test cases for the exceptions, they fail.

@BenHenning Any suggestions here as per above comment?

That's actually what I was curious about other people weighing in on. :)

I'm actually on the fence here, but given that I think we should just add the check & a new exception for it.

This should probably be tracked in a separate issue since will need to retroactively add tests for this case in each classifier being tested so far.

prayutsu · 2020-10-01T03:02:48Z

@aggarwalpulkit596 @MohamedMedhat1998 Any updates on this?

MohamedMedhat1998 · 2020-10-01T10:58:51Z

@aggarwalpulkit596 @MohamedMedhat1998 Any updates on this?

@prayutsu concerning the multiple inputs tests, we still need the opinions of the others.
@rt4914 @aggarwalpulkit596 can you take a look at this comment, please?

Concerning the negative tolerance, I don't think we need them, but let's wait for the final decision from Rajat, @rt4914 can you finalize this, please?

Other than that, everything is really simple (removing some existing tests or leaving them - we can finalize this quickly), but for now, let's focus on the multiple inputs tests first.

prayutsu · 2020-10-01T11:00:48Z

@MohamedMedhat1998 Sure

prayutsu · 2020-10-01T15:39:43Z

@rt4914 @aggarwalpulkit596 @BenHenning @MohamedMedhat1998
Once we finalize the discussion, we can update the checkpoints in the PR description, which will make it easy for me to keep a check on all the points.

prayutsu · 2020-10-02T17:04:50Z

@rt4914 @aggarwalpulkit596 Can we please have a final verdict on this?

rt4914 · 2020-10-04T14:22:39Z

@aggarwalpulkit596 @MohamedMedhat1998 Any updates on this?

@prayutsu concerning the multiple inputs tests, we still need the opinions of the others.
@rt4914 @aggarwalpulkit596 can you take a look at this comment, please?

Concerning the negative tolerance, I don't think we need them, but let's wait for the final decision from Rajat, @rt4914 can you finalize this, please?

Other than that, everything is really simple (removing some existing tests or leaving them - we can finalize this quickly), but for now, let's focus on the multiple inputs tests first.

@prayutsu @MohamedMedhat1998 I have replied to all the comments addressed to me.

This PR is blocked on two points:

The tests are failing
@aggarwalpulkit596 needs to approve this.

rt4914 · 2020-10-06T02:03:17Z

Update: I have message @aggarwalpulkit596 personally to have a look at this PR. Lets wait for him.

BenHenning

Thanks @prayutsu! This is looking really good to me. I have a suggestions for a few more tests, then I think this PR will be ready for merging.

...omain/classify/rules/numericinput/NumericInputIsWithinToleranceRuleClassifierProviderTest.kt

BenHenning · 2020-10-06T18:19:17Z

...domain/classify/rules/numberinput/NumericInputIsWithinToleranceRuleClassifierProviderTest.kt

+  }
+
+  @Test
+  fun testPositiveRealAnswer_negativeRealInput_negativeRealTolerance_answerNotWithinTolerance() {


Thanks. I actually intended that we add an exception to catch this case, but I'm backing off on that. I think we should instead:

Add a test here to verify that negative tolerance fails in a case where it would have passed for positive tolerance (to verify that we properly "no-op" in this case)

@rt4914 or someone else involved with this PR should file an Oppia web issue to add validation to ensure tolerance is always >= 0

@prayutsu can you take care of (1)?

BenHenning

Thanks @prayutsu. This LGTM!

BenHenning · 2020-10-06T19:38:33Z

Given other approvals, I'm going to go ahead and merge this if all checks pass.

@rt4914 see the comment thread above when you get a chance.

prayutsu · 2020-10-06T19:39:24Z

Thanks @prayutsu. This LGTM!

@BenHenning Thanks

prayutsu · 2020-10-06T20:01:23Z

@BenHenning Can you please put a comment - "give @prayutsu 25 points" ?

BenHenning · 2020-10-06T20:21:44Z

@MohamedMedhat1998 or @aggarwalpulkit596 could you do the scoring here?

@prayutsu since I'm not an SLoP mentor, I can't actually score the PRs.

prayutsu · 2020-10-06T20:27:39Z

@MohamedMedhat1998 or @aggarwalpulkit596 could you do the scoring here?

@prayutsu since I'm not an SLoP mentor, I can't actually score the PRs.

No issues, @MohamedMedhat1998 will update this tomorrow, thanks for merging it

MohamedMedhat1998 · 2020-10-07T11:05:17Z

give @prayutsu 25 points

rt4914 · 2020-10-07T14:47:29Z

Issue filed for following comment: #1922 (comment)

oppia/oppia#10899

Added tests for NumericInputIsWithinToleranceRuleClassifierProvider

bbeddaa

prayutsu requested a review from MohamedMedhat1998 September 30, 2020 07:39

prayutsu assigned MohamedMedhat1998 Sep 30, 2020

Fix lint error

81edcb4

aggarwalpulkit596 self-assigned this Sep 30, 2020

aggarwalpulkit596 self-requested a review September 30, 2020 07:47

aggarwalpulkit596 suggested changes Sep 30, 2020

View reviewed changes

fix reviewd changes

c83cf2b

aggarwalpulkit596 suggested changes Sep 30, 2020

View reviewed changes

aggarwalpulkit596 reviewed Sep 30, 2020

View reviewed changes

fix reviewd changes

a63aeb6

MohamedMedhat1998 suggested changes Sep 30, 2020

View reviewed changes

MohamedMedhat1998 assigned prayutsu and unassigned MohamedMedhat1998 and aggarwalpulkit596 Sep 30, 2020

rt4914 self-assigned this Sep 30, 2020

prayutsu commented Sep 30, 2020

View reviewed changes

try fixing extraInput testcase

d63c581

prayutsu assigned aggarwalpulkit596 Oct 3, 2020

prayutsu removed their assignment Oct 5, 2020

prayutsu added 3 commits October 6, 2020 11:17

fix name of one testcase

7389cb5

update package Name

25ec755

fixed all the errors

2545730

prayutsu assigned BenHenning and MohamedMedhat1998 and unassigned MohamedMedhat1998 and prayutsu Oct 6, 2020

MohamedMedhat1998 approved these changes Oct 6, 2020

View reviewed changes

MohamedMedhat1998 removed their assignment Oct 6, 2020

BenHenning requested changes Oct 6, 2020

View reviewed changes

BenHenning assigned rt4914 and prayutsu and unassigned BenHenning Oct 6, 2020

prayutsu added 3 commits October 7, 2020 00:49

fix suggested changes

08458eb

fix nit

1afd01f

removed redundant test case

e954b92

prayutsu assigned BenHenning and unassigned rt4914 Oct 6, 2020

BenHenning approved these changes Oct 6, 2020

View reviewed changes

BenHenning merged commit 544578e into oppia:develop Oct 6, 2020

prayutsu deleted the throughly-test-numeric_input_is_within_tolerance_rule_classifier branch October 6, 2020 20:04

MohamedMedhat1998 mentioned this pull request Oct 7, 2020

Fix #1888: Add test for TextInputStartsWithRuleClassifierProvider #1937

Merged

8 tasks

rt4914 mentioned this pull request Oct 7, 2020

Tolerance should always be >=0 in NumericInputRulesService oppia/oppia#10899

Open

Fix #1889 : Throughly test NumericInputIsWithinToleranceRuleClassifier #1922

Fix #1889 : Throughly test NumericInputIsWithinToleranceRuleClassifier #1922

Conversation

prayutsu commented Sep 30, 2020 • edited Loading

Explanation

Checklist

prayutsu commented Sep 30, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prayutsu commented Sep 30, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prayutsu Sep 30, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prayutsu Sep 30, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MohamedMedhat1998 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prayutsu commented Sep 30, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prayutsu commented Oct 1, 2020

MohamedMedhat1998 commented Oct 1, 2020

prayutsu commented Oct 1, 2020

prayutsu commented Oct 1, 2020 • edited Loading

prayutsu commented Oct 2, 2020

rt4914 commented Oct 4, 2020

rt4914 commented Oct 6, 2020

BenHenning left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BenHenning left a comment

Choose a reason for hiding this comment

BenHenning commented Oct 6, 2020

prayutsu commented Oct 6, 2020

prayutsu commented Oct 6, 2020

BenHenning commented Oct 6, 2020

prayutsu commented Oct 6, 2020

MohamedMedhat1998 commented Oct 7, 2020

rt4914 commented Oct 7, 2020

Fix #1889 : Throughly test `NumericInputIsWithinToleranceRuleClassifier` #1922

Fix #1889 : Throughly test `NumericInputIsWithinToleranceRuleClassifier` #1922

prayutsu commented Sep 30, 2020 •

edited

Loading

prayutsu Sep 30, 2020 •

edited

Loading

prayutsu Sep 30, 2020 •

edited

Loading

prayutsu commented Oct 1, 2020 •

edited

Loading