Skip to content

Loading…

Expected exception code #86

Closed
wants to merge 2 commits into from

6 participants

@axisK

This allows for us to use the @expectedExceptionCode annotation as we would use the @dataProvider annotation by specifying a class name and a constant on that class instead of having to put a hardcoded integer as the expected code for the exception.

@whatthejeff
Collaborator

This behavior is a bit unexpected. The @dataProvider annotation doesn't really work like this. You can't store the name of a function in a class constant and then provide the name of the class constant to the @dataProvider annotation. As far as I know, none of the annotations work like that.

@axisK

That's not actually what I meant, it's similar to dataProviders in the sense that for a dataProvider I would not expect to have to put @dataProvider array( array( 'param1', 'param2' ) ) where as for @expectedExceptionCode it does not make sense to me to put @expectedExceptionCode 7001 either. By allowing for a lookup I can safely store my error codes in constants and use that instead.

@whatthejeff
Collaborator

I understand what you mean, I just don't necessarily think it's the same. @dataProvider to me indicates the name of something that provides data. In the same sense, @expectedExceptionCode indicates a code, not the name of something that provides a code.

@axisK

It makes sense to me that @expectedExceptionCode only provides a code and is not a provider as you said it still doesn't convince me that using a constant for it is a wrong approach. Take the following scenario where codes are put into constants to avoid lookups as to what codes mean.

We have exception class Exception_Validation defining code constants as follows:

const VALIDATION_INVALID_EMAIL = 6002;
const VALIDATION_INVALID_PARAM_TYPE = 6004;
const VALIDATION_EMPTY_PARAM = 6005;

We also have some test cases that expect a Exception_Validation exception to be thrown with the various code values as exception codes. When allowing constants for codes in the annotation we can quickly see what an exception code means eg. @expectedExceptionCode Exception_Validation::VALIDATION_EMPTY_PARAM instead of @expectedExceptionCode 6005, there by also making the test case more readable.
Now if we later decide to change the code values it also does not impact my test cases or code as both still refer to Exception_Validation::VALIDATION_EMPTY_PARAM

@whatthejeff
Collaborator

I understand your use case. In fact, I typically do the same thing. I just normally use test logic to do it instead of the @expectedExceptionCode annotation. I'm actually not against your idea. It's just weird for me to use that specific annotation to accomplish it.

@dharkness

It would be a nice addition (if you didn't do it already) to also accept the error constants such as E_ERROR and E_USER_WARNING. Using defined() and constant() you could check for these and any user-defined constants as well.

if (is_numeric($annotation) { ... }
elseif (strpos($annotation, '::') !== false) { ... }
elseif (defined($annotation) {
    $code = constant($annotation);
}
@axisK

Do you perhaps have a suggestion as to what a better annotation name would be?

@markstory

Adding another annotation for this seems like a waste. The code could probably be made much simpler by using ctype_digit() and constant(). If the value isn't a digit, its probably a constant and you can get the value with constant().

@lloydwatkin

Hi guys,

@dharkness seems to be suggesting something vaguely similar to what I have developed in the pull request #481. Well the solution I have provided for expectedExceptionMessage would solve his issue with some additional code, I am happy to write tests and code for this.

Cheers, Lloyd.

@edorian
Collaborator

He,

I've merged @lloydwatkin patch because it, arguably, has a little less BC issues and also allows for expecting non-class constants.

Even so looking over the patch again the implementation looks nicer (Given that it also should not trigger autoload calls)

I'll check back with Sebastian and see if we want to use

@expectedExceptionCode @Class::foo 

or

@expectedExceptionCode Class::foo 

(without the @... which might be more "expected").

Thanks a lot for the pull request discussing this! The functionally will be in phpunit 3.7

@edorian edorian closed this
@edorian edorian added a commit that referenced this pull request
@edorian edorian Removed the '@' for @expectedException{Code,Message} paramters in fav…
…or of the more natural "Classname::CONST" syntax like dicussed in #86 and added tests for namespaced constants.
f6f50ce
@greglamb greglamb pushed a commit to greglamb/phpunit that referenced this pull request
@edorian edorian Removed the '@' for @expectedException{Code,Message} paramters in fav…
…or of the more natural "Classname::CONST" syntax like dicussed in #86 and added tests for namespaced constants.
809564b
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Dec 8, 2010
  1. Adding test cases for expectedExceptionCode change

    Petrus Rademeyer committed
This page is out of date. Refresh to see the latest.
Showing with 30 additions and 0 deletions.
  1. +15 −0 PHPUnit/Util/Test.php
  2. +5 −0 Tests/Util/TestTest.php
  3. +10 −0 Tests/_files/ExceptionTest.php
View
15 PHPUnit/Util/Test.php
@@ -128,6 +128,21 @@ public static function getExpectedException($className, $methodName)
if (isset($matches[3])) {
$code = (int)$matches[3];
+ if( $code == 0 && strpos( $annotations['method']['expectedExceptionCode'][0], '::' ) !== false ) {
+ $arrTag = explode( '::', $annotations['method']['expectedExceptionCode'][0] );
+ $sPossibleClass = $arrTag[0];
+ $sPossibleConstant = $arrTag[1];
+
+ try {
+ $cls = new ReflectionClass( $sPossibleClass );
+ if ( $cls->hasConstant( $sPossibleConstant ) ) {
+ $code = (int) $cls->getConstant( $sPossibleConstant );
+ }
+ } catch ( Exception $e ) {
+ //The class could probably not be found but we'll just let the test cases continue for now.
+ }
+
+ }
}
else if (isset($annotations['method']['expectedExceptionCode'])) {
View
5 Tests/Util/TestTest.php
@@ -88,6 +88,11 @@ public function testGetExpectedException()
array('class' => 'Class', 'code' => 1234, 'message' => 'Message'),
PHPUnit_Util_Test::getExpectedException('ExceptionTest', 'testSix')
);
+
+ $this->assertEquals(
+ array('class' => 'Class', 'code' => ExceptionTest::CODE_ONE, 'message' =>''),
+ PHPUnit_Util_Test::getExpectedException('ExceptionTest', 'testSeven')
+ );
}
public function testGetProvidedDataRegEx()
View
10 Tests/_files/ExceptionTest.php
@@ -1,6 +1,8 @@
<?php
class ExceptionTest extends PHPUnit_Framework_TestCase
{
+ const CODE_ONE = 12345;
+
/**
* @expectedException FooBarBaz
*/
@@ -44,4 +46,12 @@ public function testFive()
public function testSix()
{
}
+
+ /**
+ * @expectedException Class
+ * @expectedExceptionCode ExceptionTest::CODE_ONE
+ */
+ public function testSeven()
+ {
+ }
}
Something went wrong with that request. Please try again.