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

Add name to description for named datasets #134

Merged
merged 3 commits into from
Oct 3, 2020

Conversation

sshead
Copy link
Contributor

@sshead sshead commented Jul 19, 2020

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

With named datasets, Pest does not currently include the dataset name in test output. This PR inserts the dataset name in the dataset description, so that the output of both the test result and failure messages mimic the PHPUnit format.

For example, given the test:

test('same pest style', function ($val) {
    $this->assertSame(1, $val);
})->with([
    'one' => [1],
    'two' => [2],
]);

the current output includes:

  ✓ same with (1)
  ⨯ same with (2)

  ---

  • Tests\Unit\Datasets > same with (2)

With this PR, the new output is:

  ✓ same with data set "one" (1)
  ⨯ same with data set "two" (2)

  ---

  • Tests\Unit\Datasets > same with data set "two" (2)

The PHPUnit failure output, for a test equivalent to the above, is:

1) DatasetsTest::testSame with data set "two" (2)

Copy link
Member

@owenvoke owenvoke left a comment

Choose a reason for hiding this comment

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

This looks great to me. 👍 Just wondering if there is ever a time when you would just want the name, without the list of data, probably not though. 🤔

@sshead
Copy link
Contributor Author

sshead commented Jul 19, 2020

Hmm... With the data, it can make for very long output lines - which you don't care about much if the test passed. But if a test fails, it's nice to see the data that failed, not just the name, in the context of the test suite output. And "only sometimes" would be a bigger refactor.

@owenvoke
Copy link
Member

Fair enough, that's fine. 👍 All good in my opinion.

Copy link
Member

@nunomaduro nunomaduro left a comment

Choose a reason for hiding this comment

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

@sshead Can you rebase this pull request? Also, can you add a test that actually uses the feature and prints the output to the snapshot?

@sshead
Copy link
Contributor Author

sshead commented Sep 30, 2020

Let me know if this wasn't what you were after, or if there's anything else I need to do. (I'm such a part-time "not my day job" dev that this is the first time I've ever force-pushed ... hope I didn't break the world 😅)

Copy link
Member

@nunomaduro nunomaduro left a comment

Choose a reason for hiding this comment

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

Sounds good to me, just left a comment.

'two' => [[2]],
]));

$this->assertSame('test description with data set "one" (1)', $descriptions[0]);
Copy link
Member

Choose a reason for hiding this comment

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

Can you use the expectation API here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Copy link
Member

@nunomaduro nunomaduro left a comment

Choose a reason for hiding this comment

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

This is ready to merge and tag.

@nunomaduro
Copy link
Member

@sshead Congrats, great contribution.

@nunomaduro nunomaduro merged commit ebc9690 into pestphp:master Oct 3, 2020
@sshead sshead deleted the adds-dataset-names branch October 3, 2020 21:45
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