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

Various new traits #5

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Various new traits #5

wants to merge 1 commit into from

Conversation

larowlan
Copy link
Member

  • SpinTrait for retrying in a test
  • Extra random methods (url, email, location, address)
  • Constraint violation asserts

Copy link
Contributor

@mstrelan mstrelan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, just a few nits. Also the other existing traits have README entries. It's a hassle though, do we think it's worthwhile?

Comment on lines +15 to +24
/**
* Generates a random URL.
*
* @return string
* Random URL.
*/
protected function randomUrl(): string {
$random = new Random();
return \sprintf('https://%s.com/%s', $random->name(), $random->name());
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've always felt we should use example.com as the domain name. Turns out there is now a reserved .example TLD.

Suggested change
/**
* Generates a random URL.
*
* @return string
* Random URL.
*/
protected function randomUrl(): string {
$random = new Random();
return \sprintf('https://%s.com/%s', $random->name(), $random->name());
}
/**
* Generates a random URL.
*
* @return string
* Random URL.
*/
protected function randomUrl(): string {
$random = new Random();
return \sprintf('https://%s.example/%s', $random->name(), $random->name());
}

*/
protected function randomEmail(): string {
$random = new Random();
return \sprintf('%s@%s.com', $random->name(), $random->name());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return \sprintf('%s@%s.com', $random->name(), $random->name());
return \sprintf('%s@%s.example', $random->name(), $random->name());

protected function randomLatLonPair(): string {
$lon = $this->randomPoint(-180, 180);
$lat = $this->randomPoint(-84, 84);
return \sprintf('POINT(%s %s)', $lon, $lat);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: floats are not strings

Suggested change
return \sprintf('POINT(%s %s)', $lon, $lat);
return \sprintf('POINT(%.05n %.05n)', $lon, $lat);

Comment on lines +13 to +15
* Executes a callable until it returns TRUE.
*
* Executes executing a task until a condition is met.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* Executes a callable until it returns TRUE.
*
* Executes executing a task until a condition is met.
* Executes a callable until it returns TRUE.

Comment on lines +19 to +30
* @param int $count
* (optional) Number of times to try, defaults to 10.
* @param bool $throw
* (optional) Throw, TRUE to throw if the condition is not met.
*
* @return bool
* TRUE if lambda evaluated true.
*
* @throws \Exception
* When the condition is not met.
*/
protected function spin(callable $lambda, $count = 10, $throw = TRUE): bool {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit:

Suggested change
* @param int $count
* (optional) Number of times to try, defaults to 10.
* @param bool $throw
* (optional) Throw, TRUE to throw if the condition is not met.
*
* @return bool
* TRUE if lambda evaluated true.
*
* @throws \Exception
* When the condition is not met.
*/
protected function spin(callable $lambda, $count = 10, $throw = TRUE): bool {
* @param non-negative-int $count
* (optional) Number of times to try, defaults to 10.
* @param bool $throw
* (optional) Throw, TRUE to throw if the condition is not met.
*
* @return bool
* TRUE if lambda evaluated true.
*
* @throws \Exception
* When the condition is not met.
*/
protected function spin(callable $lambda, int $count = 10, bool $throw = TRUE): bool {

Comment on lines +38 to +40
catch (\Exception $e) {
// Do nothing.
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Non-capturing catch:

Suggested change
catch (\Exception $e) {
// Do nothing.
}
catch (\Exception) {
// Do nothing.
}

}
// Max reached.
if ($throw) {
throw new \Exception(\sprintf('Condition was not met after %d attempts', $count));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be nice to have a more specific exception, maybe \Behat\Mink\Exception\ExpectationException.

Comment on lines +46 to +53
/**
* Generates a random lat/lon point.
*/
private function randomPoint(int $min, int $max): float {
$number = \mt_rand($min, $max);
$decimals = \mt_rand(1, \pow(10, 5)) / \pow(10, 5);
return \round($number + $decimals, 5);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could also require PHP 8.3 and use Randomizer::getFloat.

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 this pull request may close these issues.

2 participants