Skip to content
This repository has been archived by the owner on Apr 20, 2021. It is now read-only.

increase JSON context DX #262

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Conversation

jdeniau
Copy link
Contributor

@jdeniau jdeniau commented Sep 11, 2018

Improve error message, by telling with node is failing, the actual content and the expected value.

@OskarStark
Copy link
Contributor

Imo you should add some testcases

@jdeniau
Copy link
Contributor Author

jdeniau commented Sep 11, 2018

@OskarStark I added some tests.

It's not 100% tested, but it's better than before ;)

@@ -119,9 +128,15 @@ public function theJsonNodeShouldBeNull($node)
*/
public function theJsonNodeShouldNotBeNull($node)
{
$this->not(function () use ($node) {
Copy link
Member

Choose a reason for hiding this comment

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

Why you don’t use the not wrapper here?

Copy link
Contributor Author

@jdeniau jdeniau Sep 14, 2018

Choose a reason for hiding this comment

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

You are right, I could have.

The only "issue" I see is that all those function need to mock or instantiate Mink instance, and I'm not really confident with that for now.

If I change back to the not, I can not easily test the function.

(The error message is Mink instance has not been set on Mink context class. Have you enabled the Mink Extension?)

@jdeniau
Copy link
Contributor Author

jdeniau commented Nov 29, 2018

@sanpii up ?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants