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

Asserts: Add expectThrowable() method #5213

Merged
merged 8 commits into from Oct 18, 2018

Conversation

burned42
Copy link
Contributor

@burned42 burned42 commented Oct 3, 2018

I thought it would be quite useful to be also able to test for Errors and (possibly in future) other Throwables, too instead of just Exceptions.

So this pull request would add a new method expectThrowable() and refactor expectException() to just call this method. I also suggest marking expectException() as deprecated since I think the old method could be removed some time later (maybe with a new major release or such?). I also added some tests and updated the documentation. Please tell me what you think about this and if there are any additional steps required that I didn't know of.

With this method you can not only test Exceptions, like with expectException, but also Errors.

Signed-off-by: Bernd Stellwag <burned@zerties.org>
Signed-off-by: Bernd Stellwag <burned@zerties.org>
Signed-off-by: Bernd Stellwag <burned@zerties.org>
Signed-off-by: Bernd Stellwag <burned@zerties.org>
…hrowable instead

Signed-off-by: Bernd Stellwag <burned@zerties.org>
Signed-off-by: Bernd Stellwag <burned@zerties.org>
Signed-off-by: Bernd Stellwag <burned@zerties.org>
@DavertMik
Copy link
Member

I also suggest marking expectException() as deprecated since I think the old method could be removed some time later

I think it's ok how it is. I got used to it )

I also added some tests and updated the documentation. Please tell me what you think about this and if there are any additional steps required that I didn't know of.

Thanks. Everything looks fine, I just didn't have enough time to review it before

Copy link
Member

@DavertMik DavertMik left a comment

Choose a reason for hiding this comment

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

Minor changes required

}

/**
* @expectedException PHPUnit\Framework\AssertionFailedError
Copy link
Member

Choose a reason for hiding this comment

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

Hey, could you update the code to not use this annotation?
Because of sebastianbergmann/phpunit#3332

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, sure, thanks for pointing that out, didn't know that! :)

Fixed in f8a2b52

}

/**
* @expectedException PHPUnit\Framework\AssertionFailedError
Copy link
Member

Choose a reason for hiding this comment

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

Please don't use annotations sebastianbergmann/phpunit#3332

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in f8a2b52

}

/**
* @expectedException PHPUnit\Framework\AssertionFailedError
Copy link
Member

Choose a reason for hiding this comment

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

Please don't use annotations sebastianbergmann/phpunit#3332

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in f8a2b52


/**
* @expectedException PHPUnit\Framework\AssertionFailedError
* @expectedExceptionMessageRegExp /RuntimeException/
Copy link
Member

Choose a reason for hiding this comment

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

Please don't use annotations sebastianbergmann/phpunit#3332

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in f8a2b52

@burned42
Copy link
Contributor Author

burned42 commented Oct 17, 2018

I also suggest marking expectException() as deprecated since I think the old method could be removed some time later

I think it's ok how it is. I got used to it )

Alright, so should I then just remove those lines?
https://github.com/Codeception/Codeception/pull/5213/files#diff-214a6d3c36d25915108ad90033f96177R319
https://github.com/Codeception/Codeception/pull/5213/files#diff-b3d3622a11b4688b45ee6999ce435bc0R459

I also added some tests and updated the documentation. Please tell me what you think about this and if there are any additional steps required that I didn't know of.

Thanks. Everything looks fine, I just didn't have enough time to review it before

Thanks for reviewing :)

@DavertMik
Copy link
Member

Thanks. Let's do that!

@DavertMik DavertMik merged commit 9549e30 into Codeception:2.5 Oct 18, 2018
@burned42 burned42 deleted the add_expect_throwable branch October 11, 2019 22:23
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.

None yet

2 participants