Port assertion methods from li3_unit to core. #790

Merged
merged 1 commit into from Jan 19, 2013

Conversation

Projects
None yet
3 participants
@blainesch
Member

blainesch commented Jan 18, 2013

No description provided.

nateabele added a commit that referenced this pull request Jan 19, 2013

Merge pull request #790 from BlaineSch/feature/unitAssertPort
Port assertion methods from li3_unit to core.

@nateabele nateabele merged commit b10bb30 into UnionOfRAD:dev Jan 19, 2013

1 check passed

default The Travis build passed
Details
@jails

This comment has been minimized.

Show comment Hide comment
@jails

jails Jan 19, 2013

Contributor

Personnaly I find most of these methods a bit useless and confusing.

$this->assertGreaterThanOrEqual($expected, $actual);

Which should be greater than or equal, the $expected or the $actual ?

Imo the old way had much more sense.

$this->assertTrue($expected >= $actual);

Is there a particular reason for adding all this methods ?

Contributor

jails commented Jan 19, 2013

Personnaly I find most of these methods a bit useless and confusing.

$this->assertGreaterThanOrEqual($expected, $actual);

Which should be greater than or equal, the $expected or the $actual ?

Imo the old way had much more sense.

$this->assertTrue($expected >= $actual);

Is there a particular reason for adding all this methods ?

@blainesch

This comment has been minimized.

Show comment Hide comment
@blainesch

blainesch Jan 19, 2013

Member

@jails they are all based off of phpunit assertions

Member

blainesch commented Jan 19, 2013

@jails they are all based off of phpunit assertions

@jails

This comment has been minimized.

Show comment Hide comment
@jails

jails Jan 19, 2013

Contributor

But why adding assertRegExp which seems to conflicts with assertPattern, moreover with its own implementation ?
If we really need a compatible interface to PHPUnit why not integrating assertSame / assertNotSame or assertEquals / assertNotEquals either ?

Well imo the only usefull assert would have been assertNotIdentical and I would have left all of this as a "PHPUnit compatiblity plugin" as an official UnionOfRAD/li3_phpunit plugin.

As the saying goes: "more code, more bug", and personally I don't miss PHPUnit ;-)

Contributor

jails commented Jan 19, 2013

But why adding assertRegExp which seems to conflicts with assertPattern, moreover with its own implementation ?
If we really need a compatible interface to PHPUnit why not integrating assertSame / assertNotSame or assertEquals / assertNotEquals either ?

Well imo the only usefull assert would have been assertNotIdentical and I would have left all of this as a "PHPUnit compatiblity plugin" as an official UnionOfRAD/li3_phpunit plugin.

As the saying goes: "more code, more bug", and personally I don't miss PHPUnit ;-)

+ * @return bool
+ */
+ public function assertStringEndsWith($expected, $actual, $message = '{:message}') {
+ return $this->assert(preg_match("/$expected$/", $actual, $matches) === 1, $message, array(

This comment has been minimized.

Show comment Hide comment
@jails

jails Jan 19, 2013

Contributor

missing antiquote "/$expected$/" ?

@jails

jails Jan 19, 2013

Contributor

missing antiquote "/$expected$/" ?

+ * Will mark the test `true` if `$object` has an attribute `$attributeName`.
+ *
+ * {{{
+ * $this->assertObjectNotHasAttribute('__construct', '\ReflectionClass');

This comment has been minimized.

Show comment Hide comment
@jails

jails Jan 19, 2013

Contributor

'ReflectionClass' would be enough

@jails

jails Jan 19, 2013

Contributor

'ReflectionClass' would be enough

@jails

This comment has been minimized.

Show comment Hide comment
@jails

jails Jan 19, 2013

Contributor

Othewise, a lot of added methods should take care about Unit::_normalizeLineEndings.

Contributor

jails commented Jan 19, 2013

Othewise, a lot of added methods should take care about Unit::_normalizeLineEndings.

@blainesch

This comment has been minimized.

Show comment Hide comment
@blainesch

blainesch Jan 19, 2013

Member

I like having lots of assertions. I'd rather an error message be "32 was not greater than or equal to 5" more than "expected true and got false". Using your method you'd need to type that message up each time you used assert(5 >= 2) is very redundant. Some also contain logic that could make your tests less readable, our harder to understand.

These were already in a plugin (blainesch/li3_unit) and were used often by me and other coworkers. I just wanted to see of they'd be accepted into core from the advice of coworker.

I think most plugins shouldn't need/have generic assertions like these, but more specific ones like assertRulePass or assertRuleFail in li3_quality.

Member

blainesch commented Jan 19, 2013

I like having lots of assertions. I'd rather an error message be "32 was not greater than or equal to 5" more than "expected true and got false". Using your method you'd need to type that message up each time you used assert(5 >= 2) is very redundant. Some also contain logic that could make your tests less readable, our harder to understand.

These were already in a plugin (blainesch/li3_unit) and were used often by me and other coworkers. I just wanted to see of they'd be accepted into core from the advice of coworker.

I think most plugins shouldn't need/have generic assertions like these, but more specific ones like assertRulePass or assertRuleFail in li3_quality.

@nateabele

This comment has been minimized.

Show comment Hide comment
@nateabele

nateabele Jan 19, 2013

Member

I tend to agree with @blainesch on this one, not only because having specific assertions makes test more readable, but more importantly, higher levels of expressiveness are correlational with higher levels of abstraction.

Also, as long as we're not going out of our way, I see API compatibility with PHPUnit as a strategic advantage. My aversion to PHPUnit itself has to do with its heaviness and slowness, not its assertion methods.

Member

nateabele commented Jan 19, 2013

I tend to agree with @blainesch on this one, not only because having specific assertions makes test more readable, but more importantly, higher levels of expressiveness are correlational with higher levels of abstraction.

Also, as long as we're not going out of our way, I see API compatibility with PHPUnit as a strategic advantage. My aversion to PHPUnit itself has to do with its heaviness and slowness, not its assertion methods.

@jails

This comment has been minimized.

Show comment Hide comment
@jails

jails Jan 20, 2013

Contributor

@blainesch If tests are smalls with an explicit method name + docblock, I don't really see the benefit about having additional "somehow generic" error message. Anyway it's just a personnal opinion.

@nateabele Ok, I see what you mean. Except Atoum which use this kind of higher level of expressiveness (needed for building asserts using it's fluent interface), I am not aware of any similar things whether in ruby, python or JS, but that doen't mean they're right.

Regarding the strategic advantage yeah it would be nice, but imo the API compatibility rely more on the PHPUnit internal (i.e. mocking, stubbing, fixtures) than the asserts.

Just for my education on this, I extracted the distribution of asserts used in all ZF2 tests.

Asserts compatible with li3: (2% of total)
assertNull: 288

Asserts "wrongly" compatible with li3: (23% of total)
assertTrue: 2524
assertFalse: 1225

Asserts not compatible with li3: (56% of total)
assertEquals: 7814
assertSame: 1361
assertNotEquals: 81
assertNotSame : 59

Asserts added in this PR : (19% of total)
assertContains: 1498
assertInstanceOf: 770
assertNotContains: 477
assertInternalType: 161
assertArrayHasKey: 127
assertRegExp: 47
assertEmpty: 46
assertObjectHasAttribute: 41
assertNotNull: 30
assertArrayNotHasKey: 24
assertCount: 10
assertGreaterThan: 9
assertNotEmpty: 6
assertContainsOnly: 6
assertNotInstanceOf: 6
assertGreaterThanOrEqual: 5
assertLessThan: 2
assertLessThanOrEqual: 1
assertFileExists: 1
assertNotCount: 0
assertClassHasAttribute: 0
assertClassNotHasAttribute: 0
assertClassHasStaticAttribute: 0
assertClassNotHasStaticAttribute: 0
assertNotContainsOnly: 0
assertContainsOnlyInstancesOf: 0
assertFileEquals: 0
assertFileNotEquals: 0
assertFileNotExists: 0
assertNotInternalType: 0
assertObjectHasNotAttribute: 0
assertNotRegExp: 0
assertStringMatchesFormat: 0
assertStringNotMatchesFormat: 0
assertStringEndsWith: 0
assertStringStartsWith: 0

Of course these stats doesn't proof anything, but it looks likes the API compatibility (concerning asserts) particularly rely on the:
assertEquals/assertSame VS assertIdentitcal/assertEqual (for 56% of tests). And imo it looks like we provide here compatibility on asserts which are not so much used.

I'm not trying to question anything, just feel like we caught between two stools here.

Contributor

jails commented Jan 20, 2013

@blainesch If tests are smalls with an explicit method name + docblock, I don't really see the benefit about having additional "somehow generic" error message. Anyway it's just a personnal opinion.

@nateabele Ok, I see what you mean. Except Atoum which use this kind of higher level of expressiveness (needed for building asserts using it's fluent interface), I am not aware of any similar things whether in ruby, python or JS, but that doen't mean they're right.

Regarding the strategic advantage yeah it would be nice, but imo the API compatibility rely more on the PHPUnit internal (i.e. mocking, stubbing, fixtures) than the asserts.

Just for my education on this, I extracted the distribution of asserts used in all ZF2 tests.

Asserts compatible with li3: (2% of total)
assertNull: 288

Asserts "wrongly" compatible with li3: (23% of total)
assertTrue: 2524
assertFalse: 1225

Asserts not compatible with li3: (56% of total)
assertEquals: 7814
assertSame: 1361
assertNotEquals: 81
assertNotSame : 59

Asserts added in this PR : (19% of total)
assertContains: 1498
assertInstanceOf: 770
assertNotContains: 477
assertInternalType: 161
assertArrayHasKey: 127
assertRegExp: 47
assertEmpty: 46
assertObjectHasAttribute: 41
assertNotNull: 30
assertArrayNotHasKey: 24
assertCount: 10
assertGreaterThan: 9
assertNotEmpty: 6
assertContainsOnly: 6
assertNotInstanceOf: 6
assertGreaterThanOrEqual: 5
assertLessThan: 2
assertLessThanOrEqual: 1
assertFileExists: 1
assertNotCount: 0
assertClassHasAttribute: 0
assertClassNotHasAttribute: 0
assertClassHasStaticAttribute: 0
assertClassNotHasStaticAttribute: 0
assertNotContainsOnly: 0
assertContainsOnlyInstancesOf: 0
assertFileEquals: 0
assertFileNotEquals: 0
assertFileNotExists: 0
assertNotInternalType: 0
assertObjectHasNotAttribute: 0
assertNotRegExp: 0
assertStringMatchesFormat: 0
assertStringNotMatchesFormat: 0
assertStringEndsWith: 0
assertStringStartsWith: 0

Of course these stats doesn't proof anything, but it looks likes the API compatibility (concerning asserts) particularly rely on the:
assertEquals/assertSame VS assertIdentitcal/assertEqual (for 56% of tests). And imo it looks like we provide here compatibility on asserts which are not so much used.

I'm not trying to question anything, just feel like we caught between two stools here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment