Refactor significance heuristic tests for easier extensability #75264
Conversation
benwtrent
added
>test
elasticmachine
added
the
Team:Analytics
|
Pinging @elastic/es-analytics-geo (Team:Analytics) |
| long c = randomLong(); | ||
| long d = randomLong(); | ||
| score = heuristic.getScore(a, b, c, d); | ||
| } catch (IllegalArgumentException e) { |
There was a problem hiding this comment.
Should the test fail here if getScore throws?
There was a problem hiding this comment.
I would think so, but that would require more logic in generating the values to ensure that they don't trip any checks.
I kept it the way it was to try to make this a "refactoring only" sort of PR.
| assertThat(score, greaterThanOrEqualTo(0.0)); | ||
| } | ||
|
|
||
| public abstract void testAssertions(); |
There was a problem hiding this comment.
Needs javadoc. Abstract methods should outline what subclasses need to do to implement them correctly. I think there's a couple other undocumented abstract methods on this class that need fixed too.
| GND gnd = new GND(true); | ||
| //term is only in the subset, not at all in the other set but that is because the other set is empty. | ||
| // this should actually not happen because only terms that are in the subset are considered now, | ||
| // however, in this case the score should be 0 because a term that does not exist cannot be relevant... |
There was a problem hiding this comment.
Nitpicking, but multi-line comments should use the /* block comment style
|
|
||
| public class SignificanceHeuristicTests extends ESTestCase { | ||
| public abstract class AbstractSignificanceHeuristicTests extends ESTestCase { |
There was a problem hiding this comment.
I think a little javadoc here to explain what you'll get by implementing this class, and what you still need to hand roll for each heuristic would be helpful. In general, I'm trying to avoid undocumented abstract classes.
|
@elasticmachine update branch |
… (#75291) The significant terms heuristic tests do not lend themselves well for new heuristics being added. This commit extracts common code and builds an abstract significant heuristic test class. This way new heuristics get the common suite of tests by extending a test class.
The original PR elastic#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 elastic#75442, elastic#75561
The significant terms heuristic tests do not lend themselves well for new heuristics being added.
This commit extracts common code and builds an abstract significant heuristic test class.
This way new heuristics get the common suite of tests by extending a test class.