EQL: support multiple test outcomes for non-deterministic TOML tests#87964
Conversation
|
Pinging @elastic/es-ql (Team:QL) |
astefan
left a comment
There was a problem hiding this comment.
This looks good.
I've left some comments. Nothing major.
| "unexpected result for spec[{}] [{}] -> {} vs {}", | ||
| name, | ||
| query, | ||
| Arrays.toString(eventIds.get(0)), |
There was a problem hiding this comment.
Extract eventIds.get(0) once and re-use it.
| actual | ||
| ); | ||
| } else { | ||
| for (long[] expected : eventIds) { |
There was a problem hiding this comment.
Move boolean succeeded = false; here, because this is the if branch that needs it.
| ); | ||
|
|
||
| fail(msg); | ||
|
|
| expected_event_ids = [ | ||
| [17, 26, 16, 13, 23, 110], | ||
| [17, 26, 16, 110, 23, 33] | ||
| ] |
There was a problem hiding this comment.
Can you keep the original format, please?
17,26,16 corresponds to the first join key ["doom","redhad"] and 13,23,110 corresponds to ["farcry", "win10"]. And each pair of three ids is put on each line so that each line corresponds to the same line in the join_keys section.
So, with the updated format it should look like
| expected_event_ids = [ | |
| [17, 26, 16, 13, 23, 110], | |
| [17, 26, 16, 110, 23, 33] | |
| ] | |
| expected_event_ids = [ | |
| [17, 26, 16, | |
| 13, 23, 110], | |
| [17, 26, 16, | |
| 110, 23, 33] | |
| ] |
| @@ -3,11 +3,14 @@ name = "anyRulesWithOneDifferentNameKey" | |||
| query = ''' | |||
There was a problem hiding this comment.
At the top of this file, at least, please add a comment describing this syntax (array of arrays in case of multiple possible solutions).
|
|
||
| import java.util.List; | ||
|
|
||
| public class EqlSpecLoaderTests extends ESTestCase { |
There was a problem hiding this comment.
I think this test is not really needed. The description to be added to test_sample.toml and the tests in samples should be enough.
| private String[] tags; | ||
| private String query; | ||
| private long[] expectedEventIds; | ||
| private List<long[]> expectedEventIds; |
There was a problem hiding this comment.
Please, add a comment to this line as well, mentioning a list of possible valid results to check.
| int i = 0; | ||
| for (Object obj : arr) { | ||
| tags[i] = (String) obj; | ||
| tags[i++] = (String) obj; |
| expected, | ||
| actual | ||
| ); | ||
| if (eventIds.size() == 1) { |
There was a problem hiding this comment.
Is this case singled out just to make use of assertArrayEquals()? Otherwise using the code in the other branch should be enough, imo. The message could be customized based on list size.
There was a problem hiding this comment.
It's just because assertArrayEquals() returns a more precise error message (ie. it also points to the exact array element that differs), that is useful in that specific case, but does not make much sense when you have multiple possible results
Some EQL test cases (Sample test cases in particular) are not completely deterministic; their outcome can change based on how the data are indexed on multiple shards.
This PR allows to define multiple possible outcomes (
expected_event_ids) for a toml test case.Both the following are now valid: