Skip to content

Fix: Add test to assert unique() behaviour#233

Merged
localheinz merged 1 commit intoFakerPHP:mainfrom
localheinz:fix/test
Dec 26, 2020
Merged

Fix: Add test to assert unique() behaviour#233
localheinz merged 1 commit intoFakerPHP:mainfrom
localheinz:fix/test

Conversation

@localheinz
Copy link
Copy Markdown
Member

@localheinz localheinz commented Dec 22, 2020

This PR

  • adds tests to assert the behaviour of Generator::unique()

Related to #155.

💁‍♂️ As a user of fakerphp/faker, I would like to continue to be able to use

$faker->unique()->word();

to generate a unique word.

I don't care if it internally uses extensions or providers, but I would like to continue using this behaviour. The tests added here ensure that this is the case.

@localheinz localheinz added the bug Something isn't working label Dec 22, 2020
@codecov
Copy link
Copy Markdown

codecov bot commented Dec 22, 2020

Codecov Report

Merging #233 (53d6fce) into main (f53f67b) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##               main     #233   +/-   ##
=========================================
  Coverage     58.40%   58.40%           
  Complexity     2044     2044           
=========================================
  Files           315      315           
  Lines          5104     5104           
=========================================
  Hits           2981     2981           
  Misses         2123     2123           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f53f67b...53d6fce. Read the comment docs.

Copy link
Copy Markdown
Member

@Nyholm Nyholm left a comment

Choose a reason for hiding this comment

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

Thank you.

Im not sure we should have the "reset" argument in the future. It makes things unpredictable. But that is not stopping us from adding these tests.

$generator->addProvider(new class(...$words) {
private $words;

public function __construct(string ...$words)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why the array deconstruction and not just pass the word array?

Copy link
Copy Markdown
Member Author

@localheinz localheinz Dec 26, 2020

Choose a reason for hiding this comment

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

@bramceulemans

Because when passing an array, it would be necessary to write more code to ensure that every element of the array is a string:

/**
 * @param array<int, string> $words
 */
public function __construct(array $words)
{
    $this->words = array_values(array_map(static function (string $word): string {
        return $word;
    }, $words));
}

or

/**
 * @param array<int, string> $words
 * 
 * @throws \InvalidArgumentException
 */
public function __construct(array $words)
{
    $notString = array_filter(static function ($word): bool {
        return !is_string($word);
    }, $words);

    if ([] !== $notString) {
        throw new \InvalidArgumentException('Every word needs to be a string');
    }

    $this->words = array_values($words);
}

Using variadics here allows me not to write this code.

@localheinz localheinz self-assigned this Dec 26, 2020
@localheinz localheinz merged commit 3d69b4d into FakerPHP:main Dec 26, 2020
@localheinz
Copy link
Copy Markdown
Member Author

Thank you, @Nyholm!

@localheinz localheinz deleted the fix/test branch December 26, 2020 12:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants