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

Feature request: Add Assert::ok($message) as contrast to Assert::fail($message) #250

Closed
JanTvrdik opened this issue Sep 7, 2015 · 12 comments
Closed

Comments

@dg

This comment has been minimized.

Copy link
Member

@dg dg commented Sep 7, 2015

This is IMHO better

    /**
     * @param  callable
     * @return void
     */
    public static function noError($function)
    {
        self::error($function, array());
    }
@richard-ejem

This comment has been minimized.

Copy link
Contributor

@richard-ejem richard-ejem commented Sep 7, 2015

@dg yes that is simple and nice :) but this covers only errors, not thrown exceptions. I'd still add support for entering nice assertion message to let testers describe what should have happenned but was interrupted by an error, like this:

/**
 * Checks if the function does neither generate PHP error nor throw Exception
 * @param  callable
 * @param  string
 * @return null|\Exception
 */
public static function noError($function, $message = NULL)
{
    self::$counter++;

    set_error_handler(function ($severity, $msg, $file, $line) use (& $expected, $message) {
        if (($severity & error_reporting()) !== $severity) {
            return;
        }
        $errorStr = Helpers::errorTypeToString($severity) . ($msg ? " ($msg)" : '');
        restore_error_handler();
        Assert::fail(($message ? "$message: " : '') . "occurred error $errorStr");
    });
    try {
        call_user_func($function);
    } catch (\Exception $e) {
        restore_error_handler();
        Assert::fail(($message ? "$message: " : '') . sprintf('%s was thrown: %s (%d)',
            get_class($e), $e->getMessage(), $e->getCode()));
    }
    restore_error_handler();
}
@milo

This comment has been minimized.

Copy link
Member

@milo milo commented Sep 8, 2015

@richard-ejem The Assert::error() is sufficient. It will be consistent across all assertions, unexpected exception will be just thrown. In your case, stack trace is missing.

@Mikulas

This comment has been minimized.

Copy link
Contributor

@Mikulas Mikulas commented Sep 8, 2015

👍 for Assert::ok. Anything longer defeats the purpose as we already can write Assert::true(TRUE, $msg).

@richard-ejem

This comment has been minimized.

Copy link
Contributor

@richard-ejem richard-ejem commented Sep 8, 2015

Assert::noError is maybe more expressive, but Assert::ok is also ok :)
@milo this is all about clear semantics - for me it's ugly to use Assert::error for expecting no error. It is for cases when I test a piece of code for not throwing particular error/exception (like the example in which my method does not throw InvalidArgumentException on argument types that should be accepted), which is semantically different case than a random unexpected error breaking my test.

Maybe also adding type of error/exception that is explicitly expected to NOT occur can be also added, like Assert::noError(function() { if (false) throw new Nette\InvalidStateException; }, 'Nette\InvalidStateException', 'false is false also today'); - will provide clear test failure message when this exception occurs and full stack trace when anything other screws up.

@milo

This comment has been minimized.

Copy link
Member

@milo milo commented Sep 8, 2015

@richard-ejem I don't force you to use Assert::error(), I wrote about Asset::noError() implementaion detail.

@richard-ejem

This comment has been minimized.

Copy link
Contributor

@richard-ejem richard-ejem commented Sep 8, 2015

@milo sorry, I didn't get that. Anyway, I still think my solution covers it in a cleaner way.

milo added a commit to milo/tester that referenced this issue Sep 8, 2015
richard-ejem added a commit to richard-ejem/tester that referenced this issue Sep 12, 2015
…a piece of code does not fail with error or exception [Closes nette#250]
@milo milo closed this in e9e1cd8 Oct 13, 2015
milo added a commit that referenced this issue Oct 13, 2015
Assert: added noError() assertion [Closes #250]
@richard-ejem

This comment has been minimized.

Copy link
Contributor

@richard-ejem richard-ejem commented Oct 13, 2015

Ok, thanks for basically copying my idea into your code ;)

@milo

This comment has been minimized.

Copy link
Member

@milo milo commented Oct 13, 2015

@richard-ejem I really don't want to steal anyone's ideas or code. The solution is based on https://forum.nette.org/cs/24204-jak-testovat-ze-neco-nehodi-vyjimku#p162036 and I supposed when I opened PR #251 before yours #252 it's OK.

But if you think you should be the commit author, let me know, I'll force push it again with your authority.

@richard-ejem

This comment has been minimized.

Copy link
Contributor

@richard-ejem richard-ejem commented Oct 13, 2015

Oh sorry, I meant to write to [#254], I just opened wrong window. There you led me to fix several things and then closed the issue with your pullrequest doing basically the same. It was just a bit surprise, but never mind,don't force-push anything because of that ;)

@milo

This comment has been minimized.

Copy link
Member

@milo milo commented Oct 13, 2015

Well, hard to explain. I know Tester's code pretty well and solving the #254 by error handler was a logical way. The #257 wasn't get well, so I came back to your PR. I proposed some changes but it is very hard and time consuming to navigate the proposer. Almost every time is much faster (and thats important to me) and easier for me write a failing test and just fix the issue.

Anyway, sorry about that feeling, I'll me more careful last time.

@richard-ejem

This comment has been minimized.

Copy link
Contributor

@richard-ejem richard-ejem commented Oct 13, 2015

OK, never mind, I'll handcraft the code more carefully next time so it won't need more fixes ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.