[GitHub] [lucene-solr] dweiss commented on a change in pull request #2267: LUCENE-9707: Hunspell: check Lucene's implementation against Hunspel's test data

2021-02-02 Thread GitBox


dweiss commented on a change in pull request #2267:
URL: https://github.com/apache/lucene-solr/pull/2267#discussion_r568397883



##
File path: 
lucene/analysis/common/src/test/org/apache/lucene/analysis/hunspell/SpellCheckerTest.java
##
@@ -61,59 +61,74 @@ public void needAffixOnAffixes() throws Exception {
 doTest("needaffix5");
   }
 
+  @Test
   public void testBreak() throws Exception {
 doTest("break");
   }
 
-  public void testBreakDefault() throws Exception {
+  @Test
+  public void breakDefault() throws Exception {
 doTest("breakdefault");
   }
 
-  public void testBreakOff() throws Exception {
+  @Test
+  public void breakOff() throws Exception {
 doTest("breakoff");
   }
 
-  public void testCompoundrule() throws Exception {
+  @Test
+  public void compoundrule() throws Exception {
 doTest("compoundrule");
   }
 
-  public void testCompoundrule2() throws Exception {
+  @Test
+  public void compoundrule2() throws Exception {
 doTest("compoundrule2");
   }
 
-  public void testCompoundrule3() throws Exception {
+  @Test
+  public void compoundrule3() throws Exception {
 doTest("compoundrule3");
   }
 
-  public void testCompoundrule4() throws Exception {
+  @Test
+  public void compoundrule4() throws Exception {
 doTest("compoundrule4");
   }
 
-  public void testCompoundrule5() throws Exception {
+  @Test
+  public void compoundrule5() throws Exception {
 doTest("compoundrule5");
   }
 
-  public void testCompoundrule6() throws Exception {
+  @Test
+  public void compoundrule6() throws Exception {
 doTest("compoundrule6");
   }
 
-  public void testCompoundrule7() throws Exception {
+  @Test
+  public void compoundrule7() throws Exception {
 doTest("compoundrule7");
   }
 
-  public void testCompoundrule8() throws Exception {
+  @Test
+  public void compoundrule8() throws Exception {
 doTest("compoundrule8");
   }
 
-  public void testGermanCompounding() throws Exception {
+  @Test
+  public void germanCompounding() throws Exception {
 doTest("germancompounding");
   }
 
   protected void doTest(String name) throws Exception {
-InputStream affixStream =
-Objects.requireNonNull(getClass().getResourceAsStream(name + ".aff"), 
name);
-InputStream dictStream =
-Objects.requireNonNull(getClass().getResourceAsStream(name + ".dic"), 
name);
+checkSpellCheckerExpectations(

Review comment:
   Mhmm... let me try again then. It's weird - tried last night and got 
permission denied. Could be that I pulled your changes via https and not ssh... 
Sorry, it was late.

##
File path: 
lucene/analysis/common/src/test/org/apache/lucene/analysis/hunspell/SpellCheckerTest.java
##
@@ -61,59 +61,74 @@ public void needAffixOnAffixes() throws Exception {
 doTest("needaffix5");
   }
 
+  @Test
   public void testBreak() throws Exception {
 doTest("break");
   }
 
-  public void testBreakDefault() throws Exception {
+  @Test
+  public void breakDefault() throws Exception {
 doTest("breakdefault");
   }
 
-  public void testBreakOff() throws Exception {
+  @Test
+  public void breakOff() throws Exception {
 doTest("breakoff");
   }
 
-  public void testCompoundrule() throws Exception {
+  @Test
+  public void compoundrule() throws Exception {
 doTest("compoundrule");
   }
 
-  public void testCompoundrule2() throws Exception {
+  @Test
+  public void compoundrule2() throws Exception {
 doTest("compoundrule2");
   }
 
-  public void testCompoundrule3() throws Exception {
+  @Test
+  public void compoundrule3() throws Exception {
 doTest("compoundrule3");
   }
 
-  public void testCompoundrule4() throws Exception {
+  @Test
+  public void compoundrule4() throws Exception {
 doTest("compoundrule4");
   }
 
-  public void testCompoundrule5() throws Exception {
+  @Test
+  public void compoundrule5() throws Exception {
 doTest("compoundrule5");
   }
 
-  public void testCompoundrule6() throws Exception {
+  @Test
+  public void compoundrule6() throws Exception {
 doTest("compoundrule6");
   }
 
-  public void testCompoundrule7() throws Exception {
+  @Test
+  public void compoundrule7() throws Exception {
 doTest("compoundrule7");
   }
 
-  public void testCompoundrule8() throws Exception {
+  @Test
+  public void compoundrule8() throws Exception {
 doTest("compoundrule8");
   }
 
-  public void testGermanCompounding() throws Exception {
+  @Test
+  public void germanCompounding() throws Exception {
 doTest("germancompounding");
   }
 
   protected void doTest(String name) throws Exception {
-InputStream affixStream =
-Objects.requireNonNull(getClass().getResourceAsStream(name + ".aff"), 
name);
-InputStream dictStream =
-Objects.requireNonNull(getClass().getResourceAsStream(name + ".dic"), 
name);
+checkSpellCheckerExpectations(

Review comment:
   Renaming SpellChecker to Hunspell - yes, I think  it's a good idea. 
Renaming 

[GitHub] [lucene-solr] dweiss commented on a change in pull request #2267: LUCENE-9707: Hunspell: check Lucene's implementation against Hunspel's test data

2021-02-02 Thread GitBox


dweiss commented on a change in pull request #2267:
URL: https://github.com/apache/lucene-solr/pull/2267#discussion_r568415196



##
File path: 
lucene/analysis/common/src/test/org/apache/lucene/analysis/hunspell/TestsFromOriginalHunspellRepository.java
##
@@ -0,0 +1,71 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.lucene.analysis.hunspell;
+
+import java.io.IOException;
+import java.nio.file.DirectoryStream;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.text.ParseException;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.Set;
+import java.util.TreeSet;
+import java.util.stream.Collectors;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
+
+/**
+ * Same as {@link SpellCheckerTest}, but checks all Hunspell's test data. The 
path to the checked
+ * out Hunspell repository should be in {@code -Dhunspell.repo.path=...} 
system property.
+ */
+@RunWith(Parameterized.class)

Review comment:
   I think they should reside in the repo if they are useful (even for 
local launches). What I'm afraid of is that if these tools are not in use, 
they'll eventually degrade and stop working without anyone noticing. 
   
   I think the way to integrate such tests properly would be to add a specific 
gradle test task which would configure an appropriate policy, require pointers 
to the required resources, etc. This way these tests can be run as a CI run 
(somewhere... maybe a github action, even?).
   
   I think this can be ironed out later on, once you've written (notice the 
'you' here... ;) more of such tests - the patterns of making them work with the 
CI will naturally emerge from that.
   
   For now, feel free to use that original parameterized test runner - I'll 
look into making IntelliJ work with randomizedtesting again (because I use it 
here and in other projects). It's a moving target and thus a bit discouraging 
(I did the same thing a few times in the past already for various IDEs that 
interpreted test descriptions differently).





This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



-
To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org
For additional commands, e-mail: issues-h...@lucene.apache.org



[GitHub] [lucene-solr] dweiss commented on a change in pull request #2267: LUCENE-9707: Hunspell: check Lucene's implementation against Hunspel's test data

2021-02-02 Thread GitBox


dweiss commented on a change in pull request #2267:
URL: https://github.com/apache/lucene-solr/pull/2267#discussion_r568398245



##
File path: 
lucene/analysis/common/src/test/org/apache/lucene/analysis/hunspell/SpellCheckerTest.java
##
@@ -61,59 +61,74 @@ public void needAffixOnAffixes() throws Exception {
 doTest("needaffix5");
   }
 
+  @Test
   public void testBreak() throws Exception {
 doTest("break");
   }
 
-  public void testBreakDefault() throws Exception {
+  @Test
+  public void breakDefault() throws Exception {
 doTest("breakdefault");
   }
 
-  public void testBreakOff() throws Exception {
+  @Test
+  public void breakOff() throws Exception {
 doTest("breakoff");
   }
 
-  public void testCompoundrule() throws Exception {
+  @Test
+  public void compoundrule() throws Exception {
 doTest("compoundrule");
   }
 
-  public void testCompoundrule2() throws Exception {
+  @Test
+  public void compoundrule2() throws Exception {
 doTest("compoundrule2");
   }
 
-  public void testCompoundrule3() throws Exception {
+  @Test
+  public void compoundrule3() throws Exception {
 doTest("compoundrule3");
   }
 
-  public void testCompoundrule4() throws Exception {
+  @Test
+  public void compoundrule4() throws Exception {
 doTest("compoundrule4");
   }
 
-  public void testCompoundrule5() throws Exception {
+  @Test
+  public void compoundrule5() throws Exception {
 doTest("compoundrule5");
   }
 
-  public void testCompoundrule6() throws Exception {
+  @Test
+  public void compoundrule6() throws Exception {
 doTest("compoundrule6");
   }
 
-  public void testCompoundrule7() throws Exception {
+  @Test
+  public void compoundrule7() throws Exception {
 doTest("compoundrule7");
   }
 
-  public void testCompoundrule8() throws Exception {
+  @Test
+  public void compoundrule8() throws Exception {
 doTest("compoundrule8");
   }
 
-  public void testGermanCompounding() throws Exception {
+  @Test
+  public void germanCompounding() throws Exception {
 doTest("germancompounding");
   }
 
   protected void doTest(String name) throws Exception {
-InputStream affixStream =
-Objects.requireNonNull(getClass().getResourceAsStream(name + ".aff"), 
name);
-InputStream dictStream =
-Objects.requireNonNull(getClass().getResourceAsStream(name + ".dic"), 
name);
+checkSpellCheckerExpectations(

Review comment:
   Renaming SpellChecker to Hunspell - yes, I think  it's a good idea. 
Renaming tests later - absolutely.





This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



-
To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org
For additional commands, e-mail: issues-h...@lucene.apache.org



[GitHub] [lucene-solr] dweiss commented on a change in pull request #2267: LUCENE-9707: Hunspell: check Lucene's implementation against Hunspel's test data

2021-02-02 Thread GitBox


dweiss commented on a change in pull request #2267:
URL: https://github.com/apache/lucene-solr/pull/2267#discussion_r568397883



##
File path: 
lucene/analysis/common/src/test/org/apache/lucene/analysis/hunspell/SpellCheckerTest.java
##
@@ -61,59 +61,74 @@ public void needAffixOnAffixes() throws Exception {
 doTest("needaffix5");
   }
 
+  @Test
   public void testBreak() throws Exception {
 doTest("break");
   }
 
-  public void testBreakDefault() throws Exception {
+  @Test
+  public void breakDefault() throws Exception {
 doTest("breakdefault");
   }
 
-  public void testBreakOff() throws Exception {
+  @Test
+  public void breakOff() throws Exception {
 doTest("breakoff");
   }
 
-  public void testCompoundrule() throws Exception {
+  @Test
+  public void compoundrule() throws Exception {
 doTest("compoundrule");
   }
 
-  public void testCompoundrule2() throws Exception {
+  @Test
+  public void compoundrule2() throws Exception {
 doTest("compoundrule2");
   }
 
-  public void testCompoundrule3() throws Exception {
+  @Test
+  public void compoundrule3() throws Exception {
 doTest("compoundrule3");
   }
 
-  public void testCompoundrule4() throws Exception {
+  @Test
+  public void compoundrule4() throws Exception {
 doTest("compoundrule4");
   }
 
-  public void testCompoundrule5() throws Exception {
+  @Test
+  public void compoundrule5() throws Exception {
 doTest("compoundrule5");
   }
 
-  public void testCompoundrule6() throws Exception {
+  @Test
+  public void compoundrule6() throws Exception {
 doTest("compoundrule6");
   }
 
-  public void testCompoundrule7() throws Exception {
+  @Test
+  public void compoundrule7() throws Exception {
 doTest("compoundrule7");
   }
 
-  public void testCompoundrule8() throws Exception {
+  @Test
+  public void compoundrule8() throws Exception {
 doTest("compoundrule8");
   }
 
-  public void testGermanCompounding() throws Exception {
+  @Test
+  public void germanCompounding() throws Exception {
 doTest("germancompounding");
   }
 
   protected void doTest(String name) throws Exception {
-InputStream affixStream =
-Objects.requireNonNull(getClass().getResourceAsStream(name + ".aff"), 
name);
-InputStream dictStream =
-Objects.requireNonNull(getClass().getResourceAsStream(name + ".dic"), 
name);
+checkSpellCheckerExpectations(

Review comment:
   Mhmm... let me try again then. It's weird - tried last night and got 
permission denied. Could be that I pulled your changes via https and not ssh... 
Sorry, it was late.





This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



-
To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org
For additional commands, e-mail: issues-h...@lucene.apache.org



[GitHub] [lucene-solr] dweiss commented on a change in pull request #2267: LUCENE-9707: Hunspell: check Lucene's implementation against Hunspel's test data

2021-02-01 Thread GitBox


dweiss commented on a change in pull request #2267:
URL: https://github.com/apache/lucene-solr/pull/2267#discussion_r568187636



##
File path: 
lucene/analysis/common/src/test/org/apache/lucene/analysis/hunspell/SpellCheckerTest.java
##
@@ -61,59 +61,74 @@ public void needAffixOnAffixes() throws Exception {
 doTest("needaffix5");
   }
 
+  @Test
   public void testBreak() throws Exception {
 doTest("break");
   }
 
-  public void testBreakDefault() throws Exception {
+  @Test
+  public void breakDefault() throws Exception {
 doTest("breakdefault");
   }
 
-  public void testBreakOff() throws Exception {
+  @Test
+  public void breakOff() throws Exception {
 doTest("breakoff");
   }
 
-  public void testCompoundrule() throws Exception {
+  @Test
+  public void compoundrule() throws Exception {
 doTest("compoundrule");
   }
 
-  public void testCompoundrule2() throws Exception {
+  @Test
+  public void compoundrule2() throws Exception {
 doTest("compoundrule2");
   }
 
-  public void testCompoundrule3() throws Exception {
+  @Test
+  public void compoundrule3() throws Exception {
 doTest("compoundrule3");
   }
 
-  public void testCompoundrule4() throws Exception {
+  @Test
+  public void compoundrule4() throws Exception {
 doTest("compoundrule4");
   }
 
-  public void testCompoundrule5() throws Exception {
+  @Test
+  public void compoundrule5() throws Exception {
 doTest("compoundrule5");
   }
 
-  public void testCompoundrule6() throws Exception {
+  @Test
+  public void compoundrule6() throws Exception {
 doTest("compoundrule6");
   }
 
-  public void testCompoundrule7() throws Exception {
+  @Test
+  public void compoundrule7() throws Exception {
 doTest("compoundrule7");
   }
 
-  public void testCompoundrule8() throws Exception {
+  @Test
+  public void compoundrule8() throws Exception {
 doTest("compoundrule8");
   }
 
-  public void testGermanCompounding() throws Exception {
+  @Test
+  public void germanCompounding() throws Exception {
 doTest("germancompounding");
   }
 
   protected void doTest(String name) throws Exception {
-InputStream affixStream =
-Objects.requireNonNull(getClass().getResourceAsStream(name + ".aff"), 
name);
-InputStream dictStream =
-Objects.requireNonNull(getClass().getResourceAsStream(name + ".dic"), 
name);
+checkSpellCheckerExpectations(

Review comment:
   Ah... can't push to your repo (there is a checkbox to enable committers 
to do so - please use it, makes edits easier :). Here is the commit:
   
   
https://github.com/dweiss/lucene-solr/commit/618a2d3b5bb51eb0e35322a9c56b97bdce7d728b





This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



-
To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org
For additional commands, e-mail: issues-h...@lucene.apache.org



[GitHub] [lucene-solr] dweiss commented on a change in pull request #2267: LUCENE-9707: Hunspell: check Lucene's implementation against Hunspel's test data

2021-02-01 Thread GitBox


dweiss commented on a change in pull request #2267:
URL: https://github.com/apache/lucene-solr/pull/2267#discussion_r568186778



##
File path: 
lucene/analysis/common/src/test/org/apache/lucene/analysis/hunspell/SpellCheckerTest.java
##
@@ -61,59 +61,74 @@ public void needAffixOnAffixes() throws Exception {
 doTest("needaffix5");
   }
 
+  @Test
   public void testBreak() throws Exception {
 doTest("break");
   }
 
-  public void testBreakDefault() throws Exception {
+  @Test
+  public void breakDefault() throws Exception {
 doTest("breakdefault");
   }
 
-  public void testBreakOff() throws Exception {
+  @Test
+  public void breakOff() throws Exception {
 doTest("breakoff");
   }
 
-  public void testCompoundrule() throws Exception {
+  @Test
+  public void compoundrule() throws Exception {
 doTest("compoundrule");
   }
 
-  public void testCompoundrule2() throws Exception {
+  @Test
+  public void compoundrule2() throws Exception {
 doTest("compoundrule2");
   }
 
-  public void testCompoundrule3() throws Exception {
+  @Test
+  public void compoundrule3() throws Exception {
 doTest("compoundrule3");
   }
 
-  public void testCompoundrule4() throws Exception {
+  @Test
+  public void compoundrule4() throws Exception {
 doTest("compoundrule4");
   }
 
-  public void testCompoundrule5() throws Exception {
+  @Test
+  public void compoundrule5() throws Exception {
 doTest("compoundrule5");
   }
 
-  public void testCompoundrule6() throws Exception {
+  @Test
+  public void compoundrule6() throws Exception {
 doTest("compoundrule6");
   }
 
-  public void testCompoundrule7() throws Exception {
+  @Test
+  public void compoundrule7() throws Exception {
 doTest("compoundrule7");
   }
 
-  public void testCompoundrule8() throws Exception {
+  @Test
+  public void compoundrule8() throws Exception {
 doTest("compoundrule8");
   }
 
-  public void testGermanCompounding() throws Exception {
+  @Test
+  public void germanCompounding() throws Exception {
 doTest("germancompounding");
   }
 
   protected void doTest(String name) throws Exception {
-InputStream affixStream =
-Objects.requireNonNull(getClass().getResourceAsStream(name + ".aff"), 
name);
-InputStream dictStream =
-Objects.requireNonNull(getClass().getResourceAsStream(name + ".dic"), 
name);
+checkSpellCheckerExpectations(

Review comment:
   You can't really convert resource URLs to paths with url.getPath. This 
breaks, as I suspected. On Windows you get:
   ```
   java.nio.file.InvalidPathException: Illegal char <:> at index 2: 
/C:/Work/apache/lucene/lucene.master/lucene/analysis/common/build/classes/java/test/org/apache/lucene/analysis/hunspell/i53643.aff
  > at 
__randomizedtesting.SeedInfo.seed([FE61D482FAEDBB53:CE18D8B46A2785A8]:0)
  > at 
java.base/sun.nio.fs.WindowsPathParser.normalize(WindowsPathParser.java:182)
  > at 
java.base/sun.nio.fs.WindowsPathParser.parse(WindowsPathParser.java:153)
  > at 
java.base/sun.nio.fs.WindowsPathParser.parse(WindowsPathParser.java:77)
  > at java.base/sun.nio.fs.WindowsPath.parse(WindowsPath.java:92)
  > at 
java.base/sun.nio.fs.WindowsFileSystem.getPath(WindowsFileSystem.java:229)
  > at java.base/java.nio.file.Path.of(Path.java:147)
   ```
   
   A better method is to go through the URI - Path.of(url.toUri()). I've 
modified the code slightly, please take a look.  
   
   Also, can you rename tests to follow TestXXX convention? This may be 
enforced in the future and will spare somebody some work to rename.





This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



-
To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org
For additional commands, e-mail: issues-h...@lucene.apache.org



[GitHub] [lucene-solr] dweiss commented on a change in pull request #2267: LUCENE-9707: Hunspell: check Lucene's implementation against Hunspel's test data

2021-02-01 Thread GitBox


dweiss commented on a change in pull request #2267:
URL: https://github.com/apache/lucene-solr/pull/2267#discussion_r568176360



##
File path: 
lucene/analysis/common/src/test/org/apache/lucene/analysis/hunspell/TestsFromOriginalHunspellRepository.java
##
@@ -0,0 +1,71 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.lucene.analysis.hunspell;
+
+import java.io.IOException;
+import java.nio.file.DirectoryStream;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.text.ParseException;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.Set;
+import java.util.TreeSet;
+import java.util.stream.Collectors;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
+
+/**
+ * Same as {@link SpellCheckerTest}, but checks all Hunspell's test data. The 
path to the checked
+ * out Hunspell repository should be in {@code -Dhunspell.repo.path=...} 
system property.
+ */
+@RunWith(Parameterized.class)

Review comment:
   Filed an issue for myself here: 
https://github.com/randomizedtesting/randomizedtesting/issues/295.





This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



-
To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org
For additional commands, e-mail: issues-h...@lucene.apache.org



[GitHub] [lucene-solr] dweiss commented on a change in pull request #2267: LUCENE-9707: Hunspell: check Lucene's implementation against Hunspel's test data

2021-02-01 Thread GitBox


dweiss commented on a change in pull request #2267:
URL: https://github.com/apache/lucene-solr/pull/2267#discussion_r568175124



##
File path: 
lucene/analysis/common/src/test/org/apache/lucene/analysis/hunspell/TestsFromOriginalHunspellRepository.java
##
@@ -0,0 +1,71 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.lucene.analysis.hunspell;
+
+import java.io.IOException;
+import java.nio.file.DirectoryStream;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.text.ParseException;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.Set;
+import java.util.TreeSet;
+import java.util.stream.Collectors;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
+
+/**
+ * Same as {@link SpellCheckerTest}, but checks all Hunspell's test data. The 
path to the checked
+ * out Hunspell repository should be in {@code -Dhunspell.repo.path=...} 
system property.
+ */
+@RunWith(Parameterized.class)

Review comment:
   I checked intellij and parameterized tests tonight. It's what I was 
afraid of - test descriptions are emitted correctly (in my opinion) but they're 
*interepreted* differently depending on the tool (and time when you check...). 
   
   The reason why you see the class name and test method before each actual 
test is because these supposedly "hidden" elements allowed tools to go back to 
the source code of a test with an arbitrary name (if you double-click on a test 
in IntelliJ it will take you back to the test method). Relaunching of a single 
test must have changed at some point because it used to be an exact name 
filter... but now I it just reruns all tests under a test method (all parameter 
variations).
   
   It's worth mentioning that this isn't consistent even in IntelliJ itself - 
if I run a simple(r) parameterized test via IntelliJ launcher, I get this test 
suite tree:
   
   
![image](https://user-images.githubusercontent.com/199470/106523393-561b9700-64e1-11eb-9000-4c4a66117331.png)
   
   But when I run the same test via gradle launcher (from within the IDE), I 
get this tree:
   
   
![image](https://user-images.githubusercontent.com/199470/106523340-3e441300-64e1-11eb-958f-dc36ece73d66.png)
   
   I don't know if there is a way to make all the tools happy; test 
descriptions and nesting is broken in JUnit 4.x itself.
   
   Given the above, please feel free to revert back to what works for you. I'd 
name the test class TestHunspellRepositoryTestCases for clarity. Also, this 
test will not run under Lucene test framework because the security manager 
won't let you access arbitrary paths outside the build location. You'd need to 
add this to tests.policy:
   ```
   permission java.io.FilePermission "${hunspell.repo.path}${/}-", "read";
   ```
   Don't know whether it's worth it at the moment though.
   





This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



-
To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org
For additional commands, e-mail: issues-h...@lucene.apache.org



[GitHub] [lucene-solr] dweiss commented on a change in pull request #2267: LUCENE-9707: Hunspell: check Lucene's implementation against Hunspel's test data

2021-02-01 Thread GitBox


dweiss commented on a change in pull request #2267:
URL: https://github.com/apache/lucene-solr/pull/2267#discussion_r567716954



##
File path: 
lucene/analysis/common/src/test/org/apache/lucene/analysis/hunspell/TestsFromOriginalHunspellRepository.java
##
@@ -0,0 +1,71 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.lucene.analysis.hunspell;
+
+import java.io.IOException;
+import java.nio.file.DirectoryStream;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.text.ParseException;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.Set;
+import java.util.TreeSet;
+import java.util.stream.Collectors;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
+
+/**
+ * Same as {@link SpellCheckerTest}, but checks all Hunspell's test data. The 
path to the checked
+ * out Hunspell repository should be in {@code -Dhunspell.repo.path=...} 
system property.
+ */
+@RunWith(Parameterized.class)

Review comment:
   Thanks. I'll take a look later, Peter. Will get back to you.





This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



-
To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org
For additional commands, e-mail: issues-h...@lucene.apache.org



[GitHub] [lucene-solr] dweiss commented on a change in pull request #2267: LUCENE-9707: Hunspell: check Lucene's implementation against Hunspel's test data

2021-02-01 Thread GitBox


dweiss commented on a change in pull request #2267:
URL: https://github.com/apache/lucene-solr/pull/2267#discussion_r567712828



##
File path: 
lucene/analysis/common/src/test/org/apache/lucene/analysis/hunspell/SpellCheckerTest.java
##
@@ -61,59 +61,74 @@ public void needAffixOnAffixes() throws Exception {
 doTest("needaffix5");
   }
 
+  @Test
   public void testBreak() throws Exception {
 doTest("break");
   }
 
-  public void testBreakDefault() throws Exception {
+  @Test
+  public void breakDefault() throws Exception {
 doTest("breakdefault");
   }
 
-  public void testBreakOff() throws Exception {
+  @Test
+  public void breakOff() throws Exception {
 doTest("breakoff");
   }
 
-  public void testCompoundrule() throws Exception {
+  @Test
+  public void compoundrule() throws Exception {
 doTest("compoundrule");
   }
 
-  public void testCompoundrule2() throws Exception {
+  @Test
+  public void compoundrule2() throws Exception {
 doTest("compoundrule2");
   }
 
-  public void testCompoundrule3() throws Exception {
+  @Test
+  public void compoundrule3() throws Exception {
 doTest("compoundrule3");
   }
 
-  public void testCompoundrule4() throws Exception {
+  @Test
+  public void compoundrule4() throws Exception {
 doTest("compoundrule4");
   }
 
-  public void testCompoundrule5() throws Exception {
+  @Test
+  public void compoundrule5() throws Exception {
 doTest("compoundrule5");
   }
 
-  public void testCompoundrule6() throws Exception {
+  @Test
+  public void compoundrule6() throws Exception {
 doTest("compoundrule6");
   }
 
-  public void testCompoundrule7() throws Exception {
+  @Test
+  public void compoundrule7() throws Exception {
 doTest("compoundrule7");
   }
 
-  public void testCompoundrule8() throws Exception {
+  @Test
+  public void compoundrule8() throws Exception {
 doTest("compoundrule8");
   }
 
-  public void testGermanCompounding() throws Exception {
+  @Test
+  public void germanCompounding() throws Exception {
 doTest("germancompounding");
   }
 
   protected void doTest(String name) throws Exception {
-InputStream affixStream =
-Objects.requireNonNull(getClass().getResourceAsStream(name + ".aff"), 
name);
-InputStream dictStream =
-Objects.requireNonNull(getClass().getResourceAsStream(name + ".dic"), 
name);
+checkSpellCheckerExpectations(

Review comment:
   Well, this isn't too fortunate then. I'll take a look later and maybe 
come up with more concrete suggestions.





This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



-
To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org
For additional commands, e-mail: issues-h...@lucene.apache.org



[GitHub] [lucene-solr] dweiss commented on a change in pull request #2267: LUCENE-9707: Hunspell: check Lucene's implementation against Hunspel's test data

2021-02-01 Thread GitBox


dweiss commented on a change in pull request #2267:
URL: https://github.com/apache/lucene-solr/pull/2267#discussion_r567700660



##
File path: 
lucene/analysis/common/src/test/org/apache/lucene/analysis/hunspell/TestsFromOriginalHunspellRepository.java
##
@@ -0,0 +1,71 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.lucene.analysis.hunspell;
+
+import java.io.IOException;
+import java.nio.file.DirectoryStream;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.text.ParseException;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.Set;
+import java.util.TreeSet;
+import java.util.stream.Collectors;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
+
+/**
+ * Same as {@link SpellCheckerTest}, but checks all Hunspell's test data. The 
path to the checked
+ * out Hunspell repository should be in {@code -Dhunspell.repo.path=...} 
system property.
+ */
+@RunWith(Parameterized.class)

Review comment:
   Test names are essentially Descriptions in JUnit. Filtering by the same 
test description should re-run the same test. IntelliJ's behavior has been 
changing over time here and I really don't know why it's not working now... Do 
you have it on that branch? I'll take a look (later).





This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



-
To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org
For additional commands, e-mail: issues-h...@lucene.apache.org



[GitHub] [lucene-solr] dweiss commented on a change in pull request #2267: LUCENE-9707: Hunspell: check Lucene's implementation against Hunspel's test data

2021-02-01 Thread GitBox


dweiss commented on a change in pull request #2267:
URL: https://github.com/apache/lucene-solr/pull/2267#discussion_r567698107



##
File path: 
lucene/analysis/common/src/test/org/apache/lucene/analysis/hunspell/SpellCheckerTest.java
##
@@ -61,59 +61,74 @@ public void needAffixOnAffixes() throws Exception {
 doTest("needaffix5");
   }
 
+  @Test
   public void testBreak() throws Exception {
 doTest("break");
   }
 
-  public void testBreakDefault() throws Exception {
+  @Test
+  public void breakDefault() throws Exception {
 doTest("breakdefault");
   }
 
-  public void testBreakOff() throws Exception {
+  @Test
+  public void breakOff() throws Exception {
 doTest("breakoff");
   }
 
-  public void testCompoundrule() throws Exception {
+  @Test
+  public void compoundrule() throws Exception {
 doTest("compoundrule");
   }
 
-  public void testCompoundrule2() throws Exception {
+  @Test
+  public void compoundrule2() throws Exception {
 doTest("compoundrule2");
   }
 
-  public void testCompoundrule3() throws Exception {
+  @Test
+  public void compoundrule3() throws Exception {
 doTest("compoundrule3");
   }
 
-  public void testCompoundrule4() throws Exception {
+  @Test
+  public void compoundrule4() throws Exception {
 doTest("compoundrule4");
   }
 
-  public void testCompoundrule5() throws Exception {
+  @Test
+  public void compoundrule5() throws Exception {
 doTest("compoundrule5");
   }
 
-  public void testCompoundrule6() throws Exception {
+  @Test
+  public void compoundrule6() throws Exception {
 doTest("compoundrule6");
   }
 
-  public void testCompoundrule7() throws Exception {
+  @Test
+  public void compoundrule7() throws Exception {
 doTest("compoundrule7");
   }
 
-  public void testCompoundrule8() throws Exception {
+  @Test
+  public void compoundrule8() throws Exception {
 doTest("compoundrule8");
   }
 
-  public void testGermanCompounding() throws Exception {
+  @Test
+  public void germanCompounding() throws Exception {
 doTest("germancompounding");
   }
 
   protected void doTest(String name) throws Exception {
-InputStream affixStream =
-Objects.requireNonNull(getClass().getResourceAsStream(name + ".aff"), 
name);
-InputStream dictStream =
-Objects.requireNonNull(getClass().getResourceAsStream(name + ".dic"), 
name);
+checkSpellCheckerExpectations(

Review comment:
   Why is it hard? getResource returns null if a resource doesn't exist. 
getResourceAsStream returns null too.





This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



-
To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org
For additional commands, e-mail: issues-h...@lucene.apache.org



[GitHub] [lucene-solr] dweiss commented on a change in pull request #2267: LUCENE-9707: Hunspell: check Lucene's implementation against Hunspel's test data

2021-02-01 Thread GitBox


dweiss commented on a change in pull request #2267:
URL: https://github.com/apache/lucene-solr/pull/2267#discussion_r567680987



##
File path: 
lucene/analysis/common/src/test/org/apache/lucene/analysis/hunspell/TestsFromOriginalHunspellRepository.java
##
@@ -0,0 +1,71 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.lucene.analysis.hunspell;
+
+import java.io.IOException;
+import java.nio.file.DirectoryStream;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.text.ParseException;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.Set;
+import java.util.TreeSet;
+import java.util.stream.Collectors;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
+
+/**
+ * Same as {@link SpellCheckerTest}, but checks all Hunspell's test data. The 
path to the checked
+ * out Hunspell repository should be in {@code -Dhunspell.repo.path=...} 
system property.
+ */
+@RunWith(Parameterized.class)

Review comment:
   You can also assumeTrue(false) (throw an assumption exception) if the 
property is not defined. This will mark the test as ignored rather than empty?





This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



-
To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org
For additional commands, e-mail: issues-h...@lucene.apache.org



[GitHub] [lucene-solr] dweiss commented on a change in pull request #2267: LUCENE-9707: Hunspell: check Lucene's implementation against Hunspel's test data

2021-02-01 Thread GitBox


dweiss commented on a change in pull request #2267:
URL: https://github.com/apache/lucene-solr/pull/2267#discussion_r567675910



##
File path: 
lucene/analysis/common/src/test/org/apache/lucene/analysis/hunspell/TestsFromOriginalHunspellRepository.java
##
@@ -0,0 +1,71 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.lucene.analysis.hunspell;
+
+import java.io.IOException;
+import java.nio.file.DirectoryStream;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.text.ParseException;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.Set;
+import java.util.TreeSet;
+import java.util.stream.Collectors;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
+
+/**
+ * Same as {@link SpellCheckerTest}, but checks all Hunspell's test data. The 
path to the checked
+ * out Hunspell repository should be in {@code -Dhunspell.repo.path=...} 
system property.
+ */
+@RunWith(Parameterized.class)

Review comment:
   If you leave this behind, it'll just be there forever... I can convert 
it if you wish. And I think it does make sense to maybe run these tests on a CI 
at some point. Just to make sure no regressions occur.





This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



-
To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org
For additional commands, e-mail: issues-h...@lucene.apache.org



[GitHub] [lucene-solr] dweiss commented on a change in pull request #2267: LUCENE-9707: Hunspell: check Lucene's implementation against Hunspel's test data

2021-02-01 Thread GitBox


dweiss commented on a change in pull request #2267:
URL: https://github.com/apache/lucene-solr/pull/2267#discussion_r567674543



##
File path: 
lucene/analysis/common/src/test/org/apache/lucene/analysis/hunspell/SpellCheckerTest.java
##
@@ -61,59 +61,74 @@ public void needAffixOnAffixes() throws Exception {
 doTest("needaffix5");
   }
 
+  @Test
   public void testBreak() throws Exception {
 doTest("break");
   }
 
-  public void testBreakDefault() throws Exception {
+  @Test
+  public void breakDefault() throws Exception {
 doTest("breakdefault");
   }
 
-  public void testBreakOff() throws Exception {
+  @Test
+  public void breakOff() throws Exception {
 doTest("breakoff");
   }
 
-  public void testCompoundrule() throws Exception {
+  @Test
+  public void compoundrule() throws Exception {
 doTest("compoundrule");
   }
 
-  public void testCompoundrule2() throws Exception {
+  @Test
+  public void compoundrule2() throws Exception {
 doTest("compoundrule2");
   }
 
-  public void testCompoundrule3() throws Exception {
+  @Test
+  public void compoundrule3() throws Exception {
 doTest("compoundrule3");
   }
 
-  public void testCompoundrule4() throws Exception {
+  @Test
+  public void compoundrule4() throws Exception {
 doTest("compoundrule4");
   }
 
-  public void testCompoundrule5() throws Exception {
+  @Test
+  public void compoundrule5() throws Exception {
 doTest("compoundrule5");
   }
 
-  public void testCompoundrule6() throws Exception {
+  @Test
+  public void compoundrule6() throws Exception {
 doTest("compoundrule6");
   }
 
-  public void testCompoundrule7() throws Exception {
+  @Test
+  public void compoundrule7() throws Exception {
 doTest("compoundrule7");
   }
 
-  public void testCompoundrule8() throws Exception {
+  @Test
+  public void compoundrule8() throws Exception {
 doTest("compoundrule8");
   }
 
-  public void testGermanCompounding() throws Exception {
+  @Test
+  public void germanCompounding() throws Exception {
 doTest("germancompounding");
   }
 
   protected void doTest(String name) throws Exception {
-InputStream affixStream =
-Objects.requireNonNull(getClass().getResourceAsStream(name + ".aff"), 
name);
-InputStream dictStream =
-Objects.requireNonNull(getClass().getResourceAsStream(name + ".dic"), 
name);
+checkSpellCheckerExpectations(

Review comment:
   (don't know if any utility method like that exists).





This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



-
To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org
For additional commands, e-mail: issues-h...@lucene.apache.org



[GitHub] [lucene-solr] dweiss commented on a change in pull request #2267: LUCENE-9707: Hunspell: check Lucene's implementation against Hunspel's test data

2021-02-01 Thread GitBox


dweiss commented on a change in pull request #2267:
URL: https://github.com/apache/lucene-solr/pull/2267#discussion_r567674247



##
File path: 
lucene/analysis/common/src/test/org/apache/lucene/analysis/hunspell/SpellCheckerTest.java
##
@@ -61,59 +61,74 @@ public void needAffixOnAffixes() throws Exception {
 doTest("needaffix5");
   }
 
+  @Test
   public void testBreak() throws Exception {
 doTest("break");
   }
 
-  public void testBreakDefault() throws Exception {
+  @Test
+  public void breakDefault() throws Exception {
 doTest("breakdefault");
   }
 
-  public void testBreakOff() throws Exception {
+  @Test
+  public void breakOff() throws Exception {
 doTest("breakoff");
   }
 
-  public void testCompoundrule() throws Exception {
+  @Test
+  public void compoundrule() throws Exception {
 doTest("compoundrule");
   }
 
-  public void testCompoundrule2() throws Exception {
+  @Test
+  public void compoundrule2() throws Exception {
 doTest("compoundrule2");
   }
 
-  public void testCompoundrule3() throws Exception {
+  @Test
+  public void compoundrule3() throws Exception {
 doTest("compoundrule3");
   }
 
-  public void testCompoundrule4() throws Exception {
+  @Test
+  public void compoundrule4() throws Exception {
 doTest("compoundrule4");
   }
 
-  public void testCompoundrule5() throws Exception {
+  @Test
+  public void compoundrule5() throws Exception {
 doTest("compoundrule5");
   }
 
-  public void testCompoundrule6() throws Exception {
+  @Test
+  public void compoundrule6() throws Exception {
 doTest("compoundrule6");
   }
 
-  public void testCompoundrule7() throws Exception {
+  @Test
+  public void compoundrule7() throws Exception {
 doTest("compoundrule7");
   }
 
-  public void testCompoundrule8() throws Exception {
+  @Test
+  public void compoundrule8() throws Exception {
 doTest("compoundrule8");
   }
 
-  public void testGermanCompounding() throws Exception {
+  @Test
+  public void germanCompounding() throws Exception {
 doTest("germancompounding");
   }
 
   protected void doTest(String name) throws Exception {
-InputStream affixStream =
-Objects.requireNonNull(getClass().getResourceAsStream(name + ".aff"), 
name);
-InputStream dictStream =
-Objects.requireNonNull(getClass().getResourceAsStream(name + ".dic"), 
name);
+checkSpellCheckerExpectations(

Review comment:
   BufferedReader is the way to go!





This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



-
To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org
For additional commands, e-mail: issues-h...@lucene.apache.org



[GitHub] [lucene-solr] dweiss commented on a change in pull request #2267: LUCENE-9707: Hunspell: check Lucene's implementation against Hunspel's test data

2021-02-01 Thread GitBox


dweiss commented on a change in pull request #2267:
URL: https://github.com/apache/lucene-solr/pull/2267#discussion_r567673956



##
File path: 
lucene/analysis/common/src/test/org/apache/lucene/analysis/hunspell/SpellCheckerTest.java
##
@@ -61,59 +61,74 @@ public void needAffixOnAffixes() throws Exception {
 doTest("needaffix5");
   }
 
+  @Test

Review comment:
   Up to you. All I'm saying is you can write testFoo() and it'll run as a 
test... if you're subclassing LuceneTestCase, that is (which I suggested 
elsewhere you should be doing - it really helps to keep test runs sane, even if 
it adds some overhead).
   
   
https://github.com/apache/lucene-solr/blob/master/lucene/test-framework/src/java/org/apache/lucene/util/LuceneTestCase.java#L188





This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



-
To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org
For additional commands, e-mail: issues-h...@lucene.apache.org



[GitHub] [lucene-solr] dweiss commented on a change in pull request #2267: LUCENE-9707: Hunspell: check Lucene's implementation against Hunspel's test data

2021-02-01 Thread GitBox


dweiss commented on a change in pull request #2267:
URL: https://github.com/apache/lucene-solr/pull/2267#discussion_r567652699



##
File path: 
lucene/analysis/common/src/test/org/apache/lucene/analysis/hunspell/SpellCheckerTest.java
##
@@ -61,59 +61,74 @@ public void needAffixOnAffixes() throws Exception {
 doTest("needaffix5");
   }
 
+  @Test

Review comment:
   The test runner in Lucene will include any methods starting with "test" 
so you don't have to annotate them, unless you want to.

##
File path: 
lucene/analysis/common/src/test/org/apache/lucene/analysis/hunspell/SpellCheckerTest.java
##
@@ -61,59 +61,74 @@ public void needAffixOnAffixes() throws Exception {
 doTest("needaffix5");
   }
 
+  @Test
   public void testBreak() throws Exception {
 doTest("break");
   }
 
-  public void testBreakDefault() throws Exception {
+  @Test
+  public void breakDefault() throws Exception {
 doTest("breakdefault");
   }
 
-  public void testBreakOff() throws Exception {
+  @Test
+  public void breakOff() throws Exception {
 doTest("breakoff");
   }
 
-  public void testCompoundrule() throws Exception {
+  @Test
+  public void compoundrule() throws Exception {
 doTest("compoundrule");
   }
 
-  public void testCompoundrule2() throws Exception {
+  @Test
+  public void compoundrule2() throws Exception {
 doTest("compoundrule2");
   }
 
-  public void testCompoundrule3() throws Exception {
+  @Test
+  public void compoundrule3() throws Exception {
 doTest("compoundrule3");
   }
 
-  public void testCompoundrule4() throws Exception {
+  @Test
+  public void compoundrule4() throws Exception {
 doTest("compoundrule4");
   }
 
-  public void testCompoundrule5() throws Exception {
+  @Test
+  public void compoundrule5() throws Exception {
 doTest("compoundrule5");
   }
 
-  public void testCompoundrule6() throws Exception {
+  @Test
+  public void compoundrule6() throws Exception {
 doTest("compoundrule6");
   }
 
-  public void testCompoundrule7() throws Exception {
+  @Test
+  public void compoundrule7() throws Exception {
 doTest("compoundrule7");
   }
 
-  public void testCompoundrule8() throws Exception {
+  @Test
+  public void compoundrule8() throws Exception {
 doTest("compoundrule8");
   }
 
-  public void testGermanCompounding() throws Exception {
+  @Test
+  public void germanCompounding() throws Exception {
 doTest("germancompounding");
   }
 
   protected void doTest(String name) throws Exception {
-InputStream affixStream =
-Objects.requireNonNull(getClass().getResourceAsStream(name + ".aff"), 
name);
-InputStream dictStream =
-Objects.requireNonNull(getClass().getResourceAsStream(name + ".dic"), 
name);
+checkSpellCheckerExpectations(

Review comment:
   Can you change it back to resource stream based implementations? 
Resource paths are not required to be on the default file system and then this 
code would fail with an odd exception. Pass the name of the resource and derive 
resource streams from that.

##
File path: 
lucene/analysis/common/src/test/org/apache/lucene/analysis/hunspell/TestsFromOriginalHunspellRepository.java
##
@@ -0,0 +1,71 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.lucene.analysis.hunspell;
+
+import java.io.IOException;
+import java.nio.file.DirectoryStream;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.text.ParseException;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.Set;
+import java.util.TreeSet;
+import java.util.stream.Collectors;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
+
+/**
+ * Same as {@link SpellCheckerTest}, but checks all Hunspell's test data. The 
path to the checked
+ * out Hunspell repository should be in {@code -Dhunspell.repo.path=...} 
system property.
+ */
+@RunWith(Parameterized.class)

Review comment:
   Please run with the default runner (extend RandomizedTest or, better, 
LuceneTestCase) - it adds a number of extra checks to make sure no interference 
between tests, proper reporting, etc. If you wish to parameterize