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

Added ability to capture multiple errors on a single value in a chain #186

Conversation

alecgunnar
Copy link
Contributor

@alecgunnar alecgunnar commented Nov 10, 2016

Using the existing LazyAssertion implementation, I found in cumbersome to capture multiple assertion failure messages pertaining to a single value. With this feature, I hope that this will be alleviated.

The purpose of the feature is to allow one to capture multiple assertion failures per value by dictating the behavior of the LazyAssertion class via special method call; the method is tryAll. What it will do specifically for this feature is, it will set a flag to keep the assertion chain going even after a failure is detected with a previous assertion.

This behavior can already be achieved, however it is necessary to call that multiple times, and this leads to multiple AssertionChain objects being created. For the sake of writing cleaner code and using resources more effectively, I felt this would be a worthwhile feature to propose.

Here is a list of exactly what I have included in this pull request:

  • Added tryAll method to \Assert\LazyAssertion
  • Added appropriate unit tests
  • Updated the documentation

- Added `tryAll` method to `\Assert\LazyAssertion`
- Added appropriate unit tests
- Updated the documentation
@alecgunnar alecgunnar force-pushed the feature-capture-multiple-errors-on-single-value-in-chain branch from 907715e to 754318e Compare November 10, 2016 14:19
->that(10, 'foo')->tryAll()->integer()
->that(null, 'foo')->notEmpty()->string()
->verifyNow();
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add this test please?


    public function testCallsToThatWithTryAllWithMultipleAssertionsAllGetReported()
    {
        $this->setExpectedException('\Assert\LazyAssertionException', <<<EXC
The following 4 assertions failed:
1) foo: Value "10" is not a float.
2) foo: Provided "10" is not greater than "100".
3) foo: Value "<NULL>" is empty, but non empty value was expected.
4) foo: Value "<NULL>" expected to be string, type NULL given.

EXC
);
        \Assert\lazy()
            ->that(10, 'foo')->tryAll()->float()->greaterThan(100)
            ->that(null, 'foo')->tryAll()->notEmpty()->string()
            ->verifyNow();
    }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For sure

@rquadling
Copy link
Contributor

rquadling commented Nov 11, 2016

Also, I think the following should work also:

        \Assert\lazy()->tryAll()
            ->that(10, 'foo')->float()->greaterThan(100)
            ->that(null, 'foo')->notEmpty()->string()
            ->verifyNow();

Currently the greaterThan() and string() failures are not reported.

Supply tryAll() to lazy() and have that set the that() tryAll automatically.

All of this is a maybe.

If you've been through the thinking on this, then let me know your thoughts and we can decide what's appropriate.

@alecgunnar
Copy link
Contributor Author

Great idea, this could certainly make using this functionality easier. I will look into making it work.

@alecgunnar
Copy link
Contributor Author

@rquadling See my most recent commit for an implementation of the "lazy level" try all.

@alecgunnar alecgunnar force-pushed the feature-capture-multiple-errors-on-single-value-in-chain branch from ae1d46b to 432befc Compare November 11, 2016 17:55
@rquadling
Copy link
Contributor

Also, I'll be looking to merge #184 at some stage. If you can rebase off this PR and make sure bin/generate_method_docs.php still operates correctly, then I'm happy with this at the moment.

@alecgunnar
Copy link
Contributor Author

I will take a look at that now, and update with the results!

@alecgunnar
Copy link
Contributor Author

alecgunnar commented Nov 11, 2016

The rebase resulted in two conflicts (resolved both) and the documentation generation script appeared to go just fine, here is the script's output from my terminal:

~/D/P/assert> php bin/generate_method_docs.php
Generated /.../assert/lib/Assert/Assertion.php.
Generated /.../assert/lib/Assert/AssertionChain.php.
Generated /.../assert/lib/Assert/LazyAssertion.php.
Generated /.../assert/README.md.

Would you like me to push the rebased branch?

->verifyAll();
```

The above shows how to use this functionality to finely tune the behavior of reporting failures, but to make catching all failures even easier, you may also call ``tryAll`` before making any assertions like below. This helps to reduce method calls, and has the same behavior as above.
Copy link
Contributor

Choose a reason for hiding this comment

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

Break this line into multiple lines please.

@rquadling
Copy link
Contributor

As there are no conflicts and the docs build doesn't fail, stick with the main master. (Sorry about jumping you around).

@alecgunnar
Copy link
Contributor Author

alecgunnar commented Nov 11, 2016

No problem, thank you! (updated the line in the README too)

@rquadling
Copy link
Contributor

And (hopefully finally) rollback the Assert::xxxx calls in the documentation.

I've got that PR needing to do some more work on the doc.

@rquadling rquadling added this to the 2.6.7 milestone Nov 11, 2016
@alecgunnar
Copy link
Contributor Author

Oops, sorry about that. I forgot I generated the docs before I made that long line fix. All set!

@rquadling rquadling merged commit 1783b41 into beberlei:master Nov 11, 2016
@rquadling
Copy link
Contributor

Excellent work. Thank you. Waiting on the other PR. When in, will be released as part of V2.6.7

@alecgunnar alecgunnar deleted the feature-capture-multiple-errors-on-single-value-in-chain branch November 11, 2016 19:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants