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

Matrix datasets #303

Merged
merged 13 commits into from
Jun 10, 2021
Merged

Matrix datasets #303

merged 13 commits into from
Jun 10, 2021

Conversation

fabio-ivona
Copy link
Collaborator

@fabio-ivona fabio-ivona commented May 24, 2021

Q A
Bug fix? no
New feature? yes
Fixed tickets -

my aim is to allow a test to be written like this one:

it('uses a matrix of datasets', function($a, $b){
    expect($a * $b)->toBeNumeric();
})->with([1, 2, 3, 4])->with([5, 6, 7, 8]);


it('uses a matrix of datasets with variadic arguments', function($a, $b){
    expect($a * $b)->toBeNumeric();
})->with([1, 2, 3, 4], [5, 6, 7, 8]);

this will generate a matrix of combination of the two datasets (cartesian product)

image

this feature allows even more than two datasets:

it('uses 3D matrix of datasets', function($a, $b, $c){
    expect($a * $b * $c)->toBeNumeric();
})->with([1, 2, 3, 4])->with([5, 6])->with([7, 8]);

it('uses again a 3D matrix of datasets', function($a, $b, $c){
    expect($a * $b * $c)->toBeNumeric();
})->with([1, 2, 3, 4], [5, 6], [7, 8]);

and will output

image

obviously the original single-dataset management it is still supported

it('uses a simple datasets', function($a){
    expect($a)->toBeNumeric();
})->with([1, 2, 3, 4]);

image

It supports named datasets

it('uses named datasets', function ($firstNumber, $secondNumber, $firstLetter, $secondLetter) {
    expect($firstNumber + $secondNumber . $firstLetter . $secondLetter)->toBeString();
})->with([
    'low numbers'  => [1, 2],
    'high numbers' => [50, 60],
])->with([
    'latin letters' => ['a', 'b'],
    'greek letters' => ['alpha', 'beta'],
]);

image

as well as all other types of datasets (closures, shared, ecc. ecc.

it('uses shared datasets', function ($firstNumber, $secondNumber, $firstLetter, $secondLetter) {
    expect($firstNumber + $secondNumber . $firstLetter . $secondLetter)->toBeString();
})->with('numbers')->with('letters');
it('uses lazy datasets', function ($firstNumber, $secondNumber, $letter) {
    expect($firstNumber + $secondNumber . $letter)->toBeString();
})->with(function(){
    yield [1, 2];
    yield [3, 4];
})->with(['a', 'b']);

@nunomaduro
Copy link
Member

This sound promising - and a good feature. Be sure to add a good test suite for this.

@fabio-ivona
Copy link
Collaborator Author

Yep, I'm moving it to draft, in order to write more tests

Glad you like it!

@fabio-ivona fabio-ivona marked this pull request as draft May 24, 2021 22:44
@fabio-ivona
Copy link
Collaborator Author

fabio-ivona commented May 25, 2021

@nunomaduro I added a new set of tests, trying to replicate all test cases that were written for single datasets (both unit and feature tests)

I also added a new way to add multiple datasets

now you can use the original syntax

it('uses shared datasets', function ($firstNumber, $secondNumber, $firstLetter, $secondLetter) {
    expect($firstNumber + $secondNumber . $firstLetter . $secondLetter)->toBeString();
})->with('numbers')->with('letters');

as well using multiple arguments in a single with :

it('uses shared datasets', function ($firstNumber, $secondNumber, $firstLetter, $secondLetter) {
    expect($firstNumber + $secondNumber . $firstLetter . $secondLetter)->toBeString();
})->with('numbers', 'letters');

what do you think?

@fabio-ivona fabio-ivona marked this pull request as ready for review May 27, 2021 06:47
@fabio-ivona
Copy link
Collaborator Author

@nunomaduro just refactored a bit and reviewed my code, I think it's ready for a deep review 😄

@fabio-ivona
Copy link
Collaborator Author

@owenvoke there is a failing test, but it seems an error during the Install Dependencies step is there any way to run it again?

@owenvoke
Copy link
Member

owenvoke commented Jun 7, 2021

@fabio-ivona, I've requested a re-run of that workflow and everything seems to be fine. 👍🏻

@fabio-ivona
Copy link
Collaborator Author

@fabio-ivona, I've requested a re-run of that workflow and everything seems to be fine. 👍🏻

Thanks!

just to know, how can I speed up a review of this PR? is there any detail missing?

@fabio-ivona
Copy link
Collaborator Author

Update: merged code from master and updated test snapshots

@nunomaduro
Copy link
Member

@fabio-ivona Can you update the documentation about this?

@nunomaduro nunomaduro merged commit 95e8add into pestphp:master Jun 10, 2021
@fabio-ivona
Copy link
Collaborator Author

@fabio-ivona Can you update the documentation about this?

Sure! I'm going to work on it this night

@nunomaduro
Copy link
Member

Thank you!

@fabio-ivona
Copy link
Collaborator Author

@nunomaduro done! pestphp/docs#56

I'm not a great non-code writer, please be merciful 😄

@fabio-ivona fabio-ivona deleted the matrix-datasets branch June 11, 2021 21:35
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.

3 participants