Drop the number of default iterations in AbstractQueryTestCase to 1 #42429
Conversation
romseygeek
added
>test
|
Pinging @elastic/es-search |
romseygeek
changed the title
|
The tests that use this number are a bit hidden and this is easy to miss them if you change the behavior of a query. With a single run a lot of failures could be missed during development and I am not sure that all contributors will be aware that they should increase this value to ensure that tests are passing locally. We had a lot of CI failures due to some missing changes in this area and that was with 20 iterations so I guess that this will just increase the number of failures that we don't spot in pr checks. The number of iterations here should not impact the build time (I didn't check but I doubt that this makes a difference) so why is it needed ? |
I think |
That was the original motivation for adding this 20 repetitions. We randomize a lot of input arguments for the query builders in these tests so the space each test covers is quite large. While we run CI a lot and eventually run into the interesting edge cases, it might take a few days and then get buried in the noise. While we were doing the query refactoring these repeats helped a lot I think. Also I think (haven't measures exactly either) that this loop doesn't add much runtime to the tests compared to e.g. start/teardown of each test suite so I think its neglegible in the overall scheme of things. But thats just my 10c, don't want to hold this up if folks want to reduce it. |
From my own experience I know that this gets forgotten quite often, even from us and even more so from external developers. When CI runs into these error scenarios its usually a few hours to days later and harder to pinpoint the code change triggering the edge case failures. Keeping the loop trades a little bit of runtime overhead for the amount of time to find and fix bugs later in the game. Again, I'm not saying this is optimal, just a tradeoff. |
Maybe but we also need a good coverage on single runs. The randomization in these tests is precisely about coverage because it is difficult to write an abstract test that ensures that all form of query builders are correctly handled. If we had to write a serialization/build check for every query I am sure we would add more than one case on the same test.
Running the tests in the |
|
I remember discussions with @cbuescher on this in the past where I was disagreeing with him on this. I have changed my mind over time, for a simple reason: if you remove those 20 iterations, some edge cases become simply much more rare. I generally still use tests.iters on top of these existing 20 iterations while developing and that gives me much more confidence that there won't be surprises. This applies only to high-level REST client serialization tests etc. where the objects that we test against can change a lot from a run to another. |
|
So I also notice no real difference with 1 or 20 iterations when running: Even when setting |
|
We discussed this in FixitFriday, and agreed to close this without committing. Instead, we should try and factor out randomization into separate instance runs, both in AbstractQueryTestCase and AbstractRequest/ResponseTest in HLRC |
As discussed in #40362 (comment),
given the number of runs our randomized CI does there is very little point in having multiple
iterations within individual tests. This commit drops the default number of iterations in
AbstractQueryTestCase to 1, while leaving the loops in-place for development purposes.