Qasbfixup #514

Merged
merged 7 commits into from Jun 22, 2012

Conversation

Projects
None yet
2 participants
Contributor

jcookems commented Jun 22, 2012

No description provided.

@ogail ogail commented on the diff Jun 22, 2012

...nctional/WindowsAzure/ServiceBus/ScenarioTestBase.php
// Note: The LockToken property is controled by the server, so cannot compare it.
+ $this->assertTrue(
@ogail

ogail Jun 22, 2012

Contributor

having two different checks in one assert is not good practice, the assert should check for one condition only. Although there could be some scenarios where it's must to do that, so make sure from this.

@jcookems

jcookems Jun 22, 2012

Contributor

I agree with a slightly stricter version of what you sugguest:

Having cascading checks in one assert is not good practice

That is, something like this, where one clause if predicated on another, is bad:

assertTrue(!is_null($foo) && !is_null($foo->getFoo2()))

The piece of code in question here is checking for one concept, nullable-String, which has an or relation (null or string). It seems reasonable to me, as a single concept. I also was unable to figure out how to refactor into separate asserts.

If you have a suggestion for how to separate into two (or more) asserts, I'd be happy to hear it, but I think that would only work for an and check.

@ogail

ogail Jun 22, 2012

Contributor

If I understand you well then I think you are looking for assertEmpty which will succeed in these cases

$this->assertEmpty('');
$this->assertEmpty(null);

and fail in other cases like

$this->assertEmpty('my str');

If you wan to assert that a given string is not empty you can use assertNotEmpty

FYI PHP built-in function empty would do check for NULL/Empty string

@jcookems

jcookems Jun 22, 2012

Contributor

Almost. What I want is something like this:

switch($lockToken)
    case NULL: AssertPass
    case String type: AssertPass
    default: AssertFail

I also have an aversion to using framework-specific asserts, given that the concepts of the tests should be applicable for all Azure SDK for *. I'll leave as-in for now.

@ogail

ogail Jun 22, 2012

Contributor

aha, you got back to our popular check in the validation :)

Contributor

ogail commented Jun 22, 2012

LGTM

@ogail ogail commented on the diff Jun 22, 2012

...nctional/WindowsAzure/ServiceBus/ScenarioTestBase.php
$this->assertEquals($expectedMessage->getLabel(), $actualMessage->getLabel(), 'getLabel');
// Note: The LockLocation property is controled by the server, so cannot compare it.
+ $this->assertTrue(
@ogail

ogail Jun 22, 2012

Contributor

For these type of asserts you can use assertInternalType function like this

$this->assertInternalType('string', ' a string value');
@jcookems

jcookems Jun 22, 2012

Contributor

I try to avoid framework-specific asserts, as it makes the task of porting tests from one language to another more difficult.

jcookems merged commit ada6431 into Azure:dev Jun 22, 2012

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