-
Notifications
You must be signed in to change notification settings - Fork 353
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
test seeding and repeat #134
Conversation
3b2b0e3
to
1f1bdd8
Compare
af11360
to
3f2672e
Compare
Can we review last comments? |
# Conflicts: # .github/workflows/tests.yml # phpunit.xml.dist # test/Faker/Provider/BaseTest.php
The tests are currently failing, can you take another 👀 , please? I think that it is a good idea to expose failures in the generators by repeatedly running the tests, but perhaps With this change, we run *221800 instead of 2236 tests on GitHub Actions, and significantly increase the build time. Perhaps it would make sense to add this as a target to |
@IonBazan apparently there's another regex-dot-related bug. I'll create a PR to fix this shortly. |
I think I've fixed this now (was not quite as easy as I had hoped), so once #206 is merged we should be green here. |
Thanks @krsriq - seems that it's working fine now! @localheinz how about we add another pipeline to run these 100x tests on push only (after merge to We will still need to stabilize |
This issue has been automatically marked as stale because it has not had recent activity. It will be closed after 1 week if no further activity occurs. Thank you for your contributions. |
Any updates on this, @IonBazan ? |
@bramceulemans sorry- there is not much to update here because it's just a POC and I believe it's up to the maintainers to decide whether they want to run these checks. I just resolved the conflicts so it's ready to merge if needed. |
Opinion on this? |
I think its a fine addition but it doesnt directly mean that those implementations are always wrong. But another important thing is that builds fail until we fix those methods as well. How do we want to handle that? We can raise issues for those tests and silent the pipeline job or we just fix them directly. I am a fan of the last one |
Closing this as stale for now. |
What is the reason for this PR?
This PR tries to rerun the tests without seeding to detect ones that are not generating valid results.
Thanks to this approach, I found following issues:
regexify
does not handle[.]
properly (see Regexify dot issue #133)old and new
optional()
behavior does not always matchPersonTest
formn_MN
locale can fail on different regex collations ([А-Я]
will not match letter Ё onC
collation)BiasedTest
is failing with random seed - needs fixing?An improvement to CI (fixes Test randomness #125)
Author's checklist
Summary of changes
There is additional test matrix entry that executes each test 100 times, excluding tests tagged
seed
andexternal
(downloading external resource) to ensure they work properly with different seeds.setUp()
method in baseTestCase
class checks if current test is inseed
group and seeds random number generator with1
if needed.verbose
mode was disabled by default to limit "skipped tests" output spam in the--repeat 100
job.Review checklist