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

[Java][Number] Decrease end property of ModelResult by 1 #2461

Merged
merged 4 commits into from
Feb 5, 2021
Merged

[Java][Number] Decrease end property of ModelResult by 1 #2461

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
4036724
Decrease end value following C# and TS
Batta32 Feb 1, 2021
fa19637
Add code comment before return
Batta32 Feb 3, 2021
a410019
Separate logic to be tested correctly in NumberTest
Batta32 Feb 3, 2021
8f6a0da
Add validation for value
Batta32 Feb 3, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion ...umber/src/main/java/com/microsoft/recognizers/text/number/models/AbstractNumberModel.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,10 +51,11 @@ public List<ModelResult> parse(String query) {
SortedMap<String, Object> sortedMap = new TreeMap<String, Object>();
sortedMap.put(ResolutionKey.Value, o.getResolutionStr());

// We decreased the end property by 1 in order to keep parity with other platforms (C#/JS).
return new ModelResult(
o.getText(),
o.getStart(),
o.getStart() + o.getLength(),
o.getStart() + o.getLength() - 1,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you add a code comment before to return to specify this is to keep behaviour consistent with the other platforms?
Also, how did the *Model.json unit tests pass if this was incorrect?
Can you fix whatever issue there is there in this PR too? Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @tellarin, we already added the code comment before to return as requested.

Also, we noticed that the start and end were not validated and that's why the tests were passing with/without these changes. We updated the NumberTest following C# and JavaScript in order to validate the necessary properties divided in each type of Recognizer.

We confirmed that all the tests are correctly passing 😊.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks! I'll review soon. Are the other models correct? DateTime, Units, ...?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tellarin, all the models are validating the typeName, text and Resolution properties of the specs.

If you are okay, we can create an issue improving this behavior for all the models apart from this PR/issue in order to have this PR reviewed/merged as soon as possible 😊.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Please create the issue and a new PR. 😊

getModelTypeName(),
sortedMap
);
Expand Down
40 changes: 40 additions & 0 deletions Java/tests/src/test/java/com/microsoft/recognizers/text/tests/number/NumberTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,17 +1,23 @@
package com.microsoft.recognizers.text.tests.number;

import com.microsoft.recognizers.text.ExtendedModelResult;
import com.microsoft.recognizers.text.ModelResult;
import com.microsoft.recognizers.text.number.NumberOptions;
import com.microsoft.recognizers.text.number.NumberRecognizer;
import com.microsoft.recognizers.text.tests.AbstractTest;
import com.microsoft.recognizers.text.tests.DependencyConstants;
import com.microsoft.recognizers.text.tests.NotSupportedException;
import com.microsoft.recognizers.text.tests.TestCase;
import org.javatuples.Pair;
import org.junit.Assert;
import org.junit.AssumptionViolatedException;
import org.junit.runners.Parameterized;

import java.util.ArrayList;
import java.util.Collection;
import java.util.Collections;
import java.util.List;
import java.util.stream.IntStream;

public class NumberTest extends AbstractTest {

Expand All @@ -25,6 +31,40 @@ public NumberTest(TestCase currentCase) {
super(currentCase);
}

@Override
protected void recognizeAndAssert(TestCase currentCase) {

// parse
List<ModelResult> results = recognize(currentCase);

// assert
assertResultsNumber(currentCase, results, new ArrayList() {{ add("value");}});
}

public static <T extends ModelResult> void assertResultsNumber(TestCase currentCase, List<T> results, List<String> testResolutionKeys) {

List<ExtendedModelResult> expectedResults = readExpectedResults(ExtendedModelResult.class, currentCase.results);
Assert.assertEquals(getMessage(currentCase, "\"Result Count\""), expectedResults.size(), results.size());

IntStream.range(0, expectedResults.size())
.mapToObj(i -> Pair.with(expectedResults.get(i), results.get(i)))
.forEach(t -> {
ExtendedModelResult expected = t.getValue0();
T actual = t.getValue1();

Assert.assertEquals(getMessage(currentCase, "typeName"), expected.typeName, actual.typeName);
Assert.assertEquals(getMessage(currentCase, "text"), expected.text, actual.text);

// Number and NumberWithUnit are supported currently.
Assert.assertEquals(getMessage(currentCase, "start"), expected.start, actual.start);
Assert.assertEquals(getMessage(currentCase, "end"), expected.end, actual.end);

for (String key : testResolutionKeys) {
Assert.assertEquals(getMessage(currentCase, key), expected.resolution.get(key), actual.resolution.get(key));
}
});
}

@Override
public List<ModelResult> recognize(TestCase currentCase) {

Expand Down