Skip to content
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

Use seed() in tests? #82

Closed
krsriq opened this issue Nov 29, 2020 · 4 comments
Closed

Use seed() in tests? #82

krsriq opened this issue Nov 29, 2020 · 4 comments

Comments

@krsriq
Copy link

krsriq commented Nov 29, 2020

I'd like to increase the Provider test coverage a bit. To do so I checked out the existing tests and saw different approaches taken:

some tests use seed() to ensure a certain result is returned, e.g.

public function testFirstNameMaleReturns()
{
$faker = new Generator();
$faker->addProvider(new Person($faker));
$faker->seed(1);
$this->assertEquals('Максим', $faker->firstNameMale());
}

other tests just check that a non-empty result is returned, e.g.

public function testIfFirstNameMaleCanReturnData()
{
$firstNameMale = $this->faker->firstNameMale();
$this->assertNotEmpty($firstNameMale);
}

Another approach would be to test that we're dealing with a valid string (in the case of firstNameMale()) or via regex a valid pattern.

Is there another solution I have overlooked/any other ideas? Which solution do you prefer?

@IonBazan
Copy link

Good point - I've noticed some inconsistency there too. What do you think about putting it in setUp() method in \Faker\Test\TestCase and making sure all tests are extending this class? That would reduce code duplication for:

$this->faker = new Generator();
$this->faker->seed(1);

While we are here, we could replace the second argument for mt_srand to MT_RAND_MT19937 as we already require PHP 7.1 so no need to keep the compatibility with previous versions:

public function seed($seed = null)
{
if ($seed === null) {
mt_srand();
} else {
mt_srand((int) $seed, MT_RAND_PHP);
}
}

But that would be a breaking change.

@pimjansen
Copy link

Agree on the default seed set in the setup. The MTRand for now is fine. Lets bump that in 2.x

@krsriq
Copy link
Author

krsriq commented Nov 30, 2020

The downside of seeding the random number generator for all tests would of course be that our tests would miss cases that only occur sometimes/randomly.

@IonBazan IonBazan mentioned this issue Nov 30, 2020
6 tasks
@pimjansen
Copy link

Closing this since we updated the seeding used in setup. If changes are still required feel free to raise a pr fornthis

@IonBazan IonBazan mentioned this issue Dec 9, 2020
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants