-
Notifications
You must be signed in to change notification settings - Fork 1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix TestTaxonomyFacetValueSource.testRandom #13198
base: main
Are you sure you want to change the base?
Fix TestTaxonomyFacetValueSource.testRandom #13198
Conversation
FYI, the test is fixed by doing the following instead:
|
@benwtrent I added above code and for some reason the Facet results have completely changed and did not include the children with value zero. Hence, it's passing. However, If i try with new boundary test case which I added it's failing. I am still digging into this more to see How changing |
Aren't we changing the random number generation when we add the merge policy, so we're no longer producing a failing case by chance? |
Whenever you touch the random number generator, it'll change anything down from there. Reiterate/Beast your tests to find a new offending seed (or improve the probability your change fixes the problem): |
@dweiss Thanks for the clarification, It does change the seed and hence was not able to reproduce the failure case. To increase the likelihood I switch to choosing only from two values (0,1), instead of @stefanvodita You are right! #12966 would consider the non-positive values as well, and the change that I introduced would start failing. So I revert the change that I did for aggregation and ran the boundary test (including negative 1) case for your PR, and It failed. Try to include this new boundary case and you will see it.
|
@iamsanjay - thank you for working on this! I merged #12966, which should mean the original test failure is fixed. Do you want to verify that all is working as expected now? |
I tried running below test case which will only test two values [0,1], and it's failing.
You can also use this pull request, only remove the fix in check result method, to reproduce it. The idea is to check for only 0 and 1. Because in existing case we are getting values from random.nextFloat which makes it almost impossible to generate zeroes or ones. |
Thank you for persisting @iamsanjay! I spend a bit of time on this and noticed two bugs, which should be fixed by #13287. Feel free to add the changes to your PR. I can still get some failures with your test, so I expect there's at least one other bug. Maybe you have better luck than I reproducing it with a smaller test. Edit: Looks like it fails on the existing test now. Will have to investigate. |
This PR has not had activity in the past 2 weeks, labeling it as stale. If the PR is waiting for review, notify the dev@lucene.apache.org list. Thank you for your contribution! |
Fix #13191