-
Notifications
You must be signed in to change notification settings - Fork 641
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/random seed simple #313
Conversation
FindFirstFailing and Seed attribute files allow to specify a particular seed to run a test under |
Its a bit strange, but although this seems to have fixed the I suspect we may have a difference in behavior somewhere in
|
I am not seeing the failing tests for TestRandomHugeStrings - can you rerun and perhaps send the seed failing? Ive run this several times and havent hit this issue. However, maybe this is because im using the FindFirstFailingSeed attribute - this sets NUnit.Framework.Internal.Randomizer.InitialSeed and NUnit.Framework.Internal.TestExecutionContext.CurrentContext.CurrentTest.Seed to the same seed which are not under normal conditions. RandomAnalysisString is producing the same result each time, I verified this by writing the hashed outputs to a file with the iteration and comparing them on subsequent runs. All identical. I can include this code in the pull request if you want to review? I will take a look at those other tests now (Lucene.Net.Analysis.NGram.EdgeNGramTokenizerTest::TestFullUTF8Range()/Lucene.Net.Analysis.NGram.NGramTokenizerTest::TestFullUTF8Range()) |
Seems both these tests call RandomUnicodeString which is predictably failing on the same seed. This one is solvable i think. |
There's no need. This has been patched already in #316. |
Ack I wish I had checked my email earlier, this does seem to fix the error. I had found a seed that failed but didnt understand why. I suspected IncrementToken() but clearly not the issue |
Adding this before the test runs seems to stop the tests erroring. Is this hiding something tho? If there is one random using a different seed, this shouldn't cause an issue. |
Before the test runs how? I tried adding it to the beginning of the test and it didn't seem to make a difference. I took a deep dive into this and there were several contributors causing these tests to fail.
There doesn't appear to be anything wrong with |
… are enabled. (closes apache#326, see apache#313)
The concurrency issue with As for changing the |
This addresses a fix for random failures in the TestRandomstrings and TestRandomHugeStrings. It seems that in the tight loop using the same Random variable creates some issues "randomly". This was not repeatable using a seed so my guess is it is due to the threads or some other concurrency within the subfunctions.
I did a lot of work to prove that the outputs from RandomAnalysisString where consistent. The issue was CheckAnalysisConsistency and I didnt see an obvious or even obscure reason why it would fail.
I havent run into any test fails so far but that doesnt mean there isnt one in the next run.