Significant terms test refactor for extendability (#75452)

The original PR #75264 made some test mistakes NXY Significant term heuristics have additional values that need to be set when testing basicScore properties. Additionally, previous refactor kept the abstract test class in a package that other plugins don't have access to. closes #75442, #75561
elastic · Jul 21, 2021 · 9b88db7 · 9b88db7
1 parent ea635d6
commit 9b88db7
Show file tree

Hide file tree

Showing 10 changed files with 84 additions and 62 deletions.
diff --git a/...va/org/elasticsearch/search/aggregations/bucket/terms/InternalMappedSignificantTerms.java b/...va/org/elasticsearch/search/aggregations/bucket/terms/InternalMappedSignificantTerms.java
@@ -92,7 +92,7 @@ protected long getSupersetSize() {
     }
 
     @Override
-    protected SignificanceHeuristic getSignificanceHeuristic() {
+    public SignificanceHeuristic getSignificanceHeuristic() {
         return significanceHeuristic;
     }
 

diff --git a/...rc/main/java/org/elasticsearch/search/aggregations/bucket/terms/SignificantLongTerms.java b/...rc/main/java/org/elasticsearch/search/aggregations/bucket/terms/SignificantLongTerms.java
@@ -25,11 +25,11 @@
 public class SignificantLongTerms extends InternalMappedSignificantTerms<SignificantLongTerms, SignificantLongTerms.Bucket> {
     public static final String NAME = "siglterms";
 
-    static class Bucket extends InternalSignificantTerms.Bucket<Bucket> {
+    public static class Bucket extends InternalSignificantTerms.Bucket<Bucket> {
 
         long term;
 
-        Bucket(long subsetDf, long subsetSize, long supersetDf, long supersetSize, long term, InternalAggregations aggregations,
+        public Bucket(long subsetDf, long subsetSize, long supersetDf, long supersetSize, long term, InternalAggregations aggregations,
                 DocValueFormat format, double score) {
             super(subsetDf, subsetSize, supersetDf, supersetSize, aggregations, format);
             this.term = term;

diff --git a/...rg/elasticsearch/search/aggregations/bucket/terms/SignificantTermsAggregationBuilder.java b/...rg/elasticsearch/search/aggregations/bucket/terms/SignificantTermsAggregationBuilder.java
@@ -151,7 +151,7 @@ protected boolean serializeTargetValueType(Version version) {
         return true;
     }
 
-    protected TermsAggregator.BucketCountThresholds getBucketCountThresholds() {
+    public TermsAggregator.BucketCountThresholds getBucketCountThresholds() {
         return new TermsAggregator.BucketCountThresholds(bucketCountThresholds);
     }
 

diff --git a/...est/java/org/elasticsearch/search/aggregations/bucket/terms/heuristic/ChiSquareTests.java b/...est/java/org/elasticsearch/search/aggregations/bucket/terms/heuristic/ChiSquareTests.java
@@ -8,21 +8,17 @@
 
 package org.elasticsearch.search.aggregations.bucket.terms.heuristic;
 
-import org.elasticsearch.search.aggregations.bucket.terms.AbstractSignificanceHeuristicTests;
+import org.elasticsearch.search.aggregations.bucket.AbstractNXYSignificanceHeuristicTestCase;
 
-public class ChiSquareTests extends AbstractSignificanceHeuristicTests {
-    @Override
-    protected SignificanceHeuristic getHeuristic() {
-        return new ChiSquare(randomBoolean(), randomBoolean());
-    }
+public class ChiSquareTests extends AbstractNXYSignificanceHeuristicTestCase {
 
     @Override
-    protected boolean testZeroScore() {
-        return false;
+    public void testAssertions() {
+        testBackgroundAssertions(new ChiSquare(true, true), new ChiSquare(true, false));
     }
 
     @Override
-    public void testAssertions() {
-        testBackgroundAssertions(new ChiSquare(true, true), new ChiSquare(true, false));
+    protected SignificanceHeuristic getHeuristic(boolean includeNegatives, boolean backgroundIsSuperset) {
+        return new ChiSquare(includeNegatives, backgroundIsSuperset);
     }
 }
diff --git a/.../src/test/java/org/elasticsearch/search/aggregations/bucket/terms/heuristic/GNDTests.java b/.../src/test/java/org/elasticsearch/search/aggregations/bucket/terms/heuristic/GNDTests.java
@@ -8,21 +8,16 @@
 
 package org.elasticsearch.search.aggregations.bucket.terms.heuristic;
 
-import org.elasticsearch.search.aggregations.bucket.terms.AbstractSignificanceHeuristicTests;
+import org.elasticsearch.search.aggregations.bucket.AbstractSignificanceHeuristicTestCase;
 
 import static org.hamcrest.Matchers.equalTo;
 
-public class GNDTests extends AbstractSignificanceHeuristicTests {
+public class GNDTests extends AbstractSignificanceHeuristicTestCase {
     @Override
     protected SignificanceHeuristic getHeuristic() {
         return new GND(randomBoolean());
     }
 
-    @Override
-    protected boolean testZeroScore() {
-        return true;
-    }
-
     @Override
     public void testAssertions() {
         testBackgroundAssertions(new GND(true), new GND(false));

diff --git a/...test/java/org/elasticsearch/search/aggregations/bucket/terms/heuristic/JLHScoreTests.java b/...test/java/org/elasticsearch/search/aggregations/bucket/terms/heuristic/JLHScoreTests.java
@@ -8,19 +8,14 @@
 
 package org.elasticsearch.search.aggregations.bucket.terms.heuristic;
 
-import org.elasticsearch.search.aggregations.bucket.terms.AbstractSignificanceHeuristicTests;
+import org.elasticsearch.search.aggregations.bucket.AbstractSignificanceHeuristicTestCase;
 
-public class JLHScoreTests extends AbstractSignificanceHeuristicTests {
+public class JLHScoreTests extends AbstractSignificanceHeuristicTestCase {
     @Override
     protected SignificanceHeuristic getHeuristic() {
         return new JLHScore();
     }
 
-    @Override
-    protected boolean testZeroScore() {
-        return true;
-    }
-
     @Override
     public void testAssertions() {
         testAssertions(new JLHScore());

diff --git a/.../org/elasticsearch/search/aggregations/bucket/terms/heuristic/MutualInformationTests.java b/.../org/elasticsearch/search/aggregations/bucket/terms/heuristic/MutualInformationTests.java
@@ -8,23 +8,18 @@
 
 package org.elasticsearch.search.aggregations.bucket.terms.heuristic;
 
-import org.elasticsearch.search.aggregations.bucket.terms.AbstractSignificanceHeuristicTests;
+import org.elasticsearch.search.aggregations.bucket.AbstractNXYSignificanceHeuristicTestCase;
 
 import static org.hamcrest.Matchers.equalTo;
 import static org.hamcrest.Matchers.greaterThan;
 import static org.hamcrest.Matchers.greaterThanOrEqualTo;
 import static org.hamcrest.Matchers.lessThan;
 import static org.hamcrest.Matchers.lessThanOrEqualTo;
 
-public class MutualInformationTests extends AbstractSignificanceHeuristicTests {
+public class MutualInformationTests extends AbstractNXYSignificanceHeuristicTestCase {
     @Override
-    protected SignificanceHeuristic getHeuristic() {
-        return new MutualInformation(randomBoolean(), randomBoolean());
-    }
-
-    @Override
-    protected boolean testZeroScore() {
-        return false;
+    protected SignificanceHeuristic getHeuristic(boolean includeNegatives, boolean backgroundIsSuperset) {
+        return new MutualInformation(includeNegatives, backgroundIsSuperset);
     }
 
     @Override

diff --git a/...va/org/elasticsearch/search/aggregations/bucket/terms/heuristic/PercentageScoreTests.java b/...va/org/elasticsearch/search/aggregations/bucket/terms/heuristic/PercentageScoreTests.java
@@ -8,19 +8,14 @@
 
 package org.elasticsearch.search.aggregations.bucket.terms.heuristic;
 
-import org.elasticsearch.search.aggregations.bucket.terms.AbstractSignificanceHeuristicTests;
+import org.elasticsearch.search.aggregations.bucket.AbstractSignificanceHeuristicTestCase;
 
-public class PercentageScoreTests extends AbstractSignificanceHeuristicTests {
+public class PercentageScoreTests extends AbstractSignificanceHeuristicTestCase {
     @Override
     protected SignificanceHeuristic getHeuristic() {
         return new PercentageScore();
     }
 
-    @Override
-    protected boolean testZeroScore() {
-        return true;
-    }
-
     @Override
     public void testAssertions() {
         testAssertions(new PercentageScore());

diff --git a/...rg/elasticsearch/search/aggregations/bucket/AbstractNXYSignificanceHeuristicTestCase.java b/...rg/elasticsearch/search/aggregations/bucket/AbstractNXYSignificanceHeuristicTestCase.java
@@ -0,0 +1,39 @@
+/*
+ * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
+ * or more contributor license agreements. Licensed under the Elastic License
+ * 2.0 and the Server Side Public License, v 1; you may not use this file except
+ * in compliance with, at your election, the Elastic License 2.0 or the Server
+ * Side Public License, v 1.
+ */
+
+package org.elasticsearch.search.aggregations.bucket;
+
+import org.elasticsearch.search.aggregations.bucket.terms.heuristic.SignificanceHeuristic;
+
+/**
+ * Abstract test case for testing NXY significant term heuristics
+ */
+public abstract class AbstractNXYSignificanceHeuristicTestCase extends AbstractSignificanceHeuristicTestCase {
+
+    /**
+     * @return A random instance of the heuristic to test
+     */
+    @Override
+    protected SignificanceHeuristic getHeuristic() {
+        return getHeuristic(randomBoolean(), randomBoolean());
+    }
+
+    /**
+     * @param includeNegatives value for this test run, should the scores include negative values.
+     * @param backgroundIsSuperset value for this test run, indicates in NXY significant terms if the background is indeed
+     *                             a superset of the the subset, or is instead a disjoint set
+     * @return  A random instance of an NXY heuristic to test
+     */
+    protected abstract SignificanceHeuristic getHeuristic(boolean includeNegatives, boolean backgroundIsSuperset);
+
+    @Override
+    public void testBasicScoreProperties() {
+        testBasicScoreProperties(getHeuristic(true, true), false);
+    }
+
+}
diff --git a/...s/AbstractSignificanceHeuristicTests.java → ...bstractSignificanceHeuristicTestCase.java b/...s/AbstractSignificanceHeuristicTests.java → ...bstractSignificanceHeuristicTestCase.java
@@ -6,7 +6,7 @@
  * Side Public License, v 1.
  */
 
-package org.elasticsearch.search.aggregations.bucket.terms;
+package org.elasticsearch.search.aggregations.bucket;
 
 import org.apache.lucene.util.BytesRef;
 import org.elasticsearch.Version;
@@ -28,6 +28,12 @@
 import org.elasticsearch.search.SearchModule;
 import org.elasticsearch.search.aggregations.InternalAggregation;
 import org.elasticsearch.search.aggregations.InternalAggregations;
+import org.elasticsearch.search.aggregations.bucket.terms.InternalMappedSignificantTerms;
+import org.elasticsearch.search.aggregations.bucket.terms.InternalSignificantTerms;
+import org.elasticsearch.search.aggregations.bucket.terms.SignificantLongTerms;
+import org.elasticsearch.search.aggregations.bucket.terms.SignificantStringTerms;
+import org.elasticsearch.search.aggregations.bucket.terms.SignificantTerms;
+import org.elasticsearch.search.aggregations.bucket.terms.SignificantTermsAggregationBuilder;
 import org.elasticsearch.search.aggregations.bucket.terms.heuristic.SignificanceHeuristic;
 import org.elasticsearch.test.ESTestCase;
 import org.elasticsearch.test.InternalAggregationTestCase;
@@ -56,18 +62,13 @@
 /**
  * Abstract test case for testing significant term heuristics
  */
-public abstract class AbstractSignificanceHeuristicTests extends ESTestCase {
+public abstract class AbstractSignificanceHeuristicTestCase extends ESTestCase {
 
     /**
      * @return A random instance of the heuristic to test
      */
     protected abstract SignificanceHeuristic getHeuristic();
 
-    /**
-     * @return test if the score is `0` with a subset frequency of `0`
-     */
-    protected abstract boolean testZeroScore();
-
     // test that stream output can actually be read - does not replace bwc test
     public void testStreamResponse() throws Exception {
         Version version = randomVersion(random());
@@ -83,13 +84,11 @@ public void testStreamResponse() throws Exception {
         ByteArrayInputStream inBuffer = new ByteArrayInputStream(outBuffer.toByteArray());
         StreamInput in = new InputStreamStreamInput(inBuffer);
         // populates the registry through side effects
-        SearchModule searchModule = new SearchModule(Settings.EMPTY, emptyList());
-        NamedWriteableRegistry registry = new NamedWriteableRegistry(searchModule.getNamedWriteables());
-        in = new NamedWriteableAwareStreamInput(in, registry);
+        in = new NamedWriteableAwareStreamInput(in, writableRegistry());
         in.setVersion(version);
         InternalMappedSignificantTerms<?, ?> read = (InternalMappedSignificantTerms<?, ?>) in.readNamedWriteable(InternalAggregation.class);
 
-        assertEquals(sigTerms.significanceHeuristic, read.significanceHeuristic);
+        assertEquals(sigTerms.getSignificanceHeuristic(), read.getSignificanceHeuristic());
         SignificantTerms.Bucket originalBucket = sigTerms.getBuckets().get(0);
         SignificantTerms.Bucket streamedBucket = read.getBuckets().get(0);
         assertThat(originalBucket.getKeyAsString(), equalTo(streamedBucket.getKeyAsString()));
@@ -127,11 +126,14 @@ public void testReduce() {
     }
 
     public void testBasicScoreProperties() {
-        SignificanceHeuristic heuristic = getHeuristic();
+        testBasicScoreProperties(getHeuristic(), true);
+    }
+
+    protected void testBasicScoreProperties(SignificanceHeuristic heuristic, boolean testZeroScore) {
         assertThat(heuristic.getScore(1, 1, 1, 3), greaterThan(0.0));
         assertThat(heuristic.getScore(1, 1, 2, 3), lessThan(heuristic.getScore(1, 1, 1, 3)));
         assertThat(heuristic.getScore(1, 1, 3, 4), lessThan(heuristic.getScore(1, 1, 2, 4)));
-        if (testZeroScore()) {
+        if (testZeroScore) {
             assertThat(heuristic.getScore(0, 1, 2, 3), equalTo(0.0));
         }
 
@@ -150,8 +152,8 @@ public void testBasicScoreProperties() {
     /**
      * Testing heuristic specific assertions
      * Typically, this method would call either
-     * {@link AbstractSignificanceHeuristicTests#testBackgroundAssertions(SignificanceHeuristic, SignificanceHeuristic)}
-     * or {@link AbstractSignificanceHeuristicTests#testAssertions(SignificanceHeuristic)}
+     * {@link AbstractSignificanceHeuristicTestCase#testBackgroundAssertions(SignificanceHeuristic, SignificanceHeuristic)}
+     * or {@link AbstractSignificanceHeuristicTestCase#testAssertions(SignificanceHeuristic)}
      * depending on which was appropriate
      */
     public abstract void testAssertions();
@@ -206,9 +208,9 @@ public void testParseFailure() throws IOException {
     // Create aggregations as they might come from three different shards and return as list.
     private List<InternalAggregation> createInternalAggregations() {
         SignificanceHeuristic significanceHeuristic = getHeuristic();
-        AbstractSignificanceHeuristicTests.TestAggFactory<?, ?> factory = randomBoolean() ?
-            new AbstractSignificanceHeuristicTests.StringTestAggFactory() :
-            new AbstractSignificanceHeuristicTests.LongTestAggFactory();
+        AbstractSignificanceHeuristicTestCase.TestAggFactory<?, ?> factory = randomBoolean() ?
+            new AbstractSignificanceHeuristicTestCase.StringTestAggFactory() :
+            new AbstractSignificanceHeuristicTestCase.LongTestAggFactory();
 
         List<InternalAggregation> aggs = new ArrayList<>();
         aggs.add(factory.createAggregation(significanceHeuristic, 4, 10, 1, (f, i) -> f.createBucket(4, 4, 5, 10, 0)));
@@ -273,6 +275,11 @@ protected NamedXContentRegistry xContentRegistry() {
         return new NamedXContentRegistry(new SearchModule(Settings.EMPTY, emptyList()).getNamedXContents());
     }
 
+    @Override
+    protected NamedWriteableRegistry writableRegistry() {
+        return new NamedWriteableRegistry(new SearchModule(Settings.EMPTY, emptyList()).getNamedWriteables());
+    }
+
     protected void testBackgroundAssertions(SignificanceHeuristic heuristicIsSuperset, SignificanceHeuristic heuristicNotSuperset) {
         try {
             heuristicIsSuperset.getScore(2, 3, 1, 4);