-
Notifications
You must be signed in to change notification settings - Fork 151
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
Fix: Use more appropriate assertions #37
Conversation
@@ -97,6 +97,6 @@ public function testSilentlySendTraces() | |||
$httpTransport->send($traces); | |||
$output = ob_get_clean(); | |||
|
|||
$this->assertEmpty($output); | |||
$this->assertSame('', $output); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably better to compare with an empty string, as
$this->assertEmpty('[]');
will pass.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could check strlen() also, but this is fine imo.
tests/Unit/Encoders/JsonTest.php
Outdated
@@ -28,6 +28,6 @@ public function testEncodeTracesSuccess() | |||
); | |||
$jsonEncoder = new Json(); | |||
$encodedTrace = $jsonEncoder->encodeTraces([[$span]]); | |||
$this->assertEquals($expectedPayload, $encodedTrace); | |||
$this->assertJsonStringEqualsJsonString($expectedPayload, $encodedTrace); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This assertion will also give a more helpful error message when it fails.
tests/Unit/SpanContextTest.php
Outdated
@@ -35,7 +35,7 @@ public function testGetBaggageItemsReturnsExpectedValues() | |||
self::BAGGAGE_ITEM_KEY => self::BAGGAGE_ITEM_VALUE, | |||
]); | |||
|
|||
$this->assertEquals(self::BAGGAGE_ITEM_VALUE, $context->getBaggageItem(self::BAGGAGE_ITEM_KEY)); | |||
$this->assertSame(self::BAGGAGE_ITEM_VALUE, $context->getBaggageItem(self::BAGGAGE_ITEM_KEY)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure about this change. assertSame
gives me the feeling I am comparing objects and not the value of the string which is what we aim to test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it depends on whether we care that a string
value is returned, or would be fine, for example if an object were returned that could be casted to string
.
Not sure, what do you think?
Personally, I prefer to be as strict as possible, but I'm aware you know much more about this project than I do!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with José here; being strict on types in tests when we just want a string-ish value could lead to brittleness later. Probably not in baggage items, but as a general philosophy I'd rather go that way. Kind of like accepting interfaces ... we want to get back something that acts like a string, it shouldn't be important what the actual implementation is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The json comparison, and empty string check, look good to me. The rest, unless there's a reason that we really care about type of those arguments, I think are not necessary.
No problem, will adjust later when I’m back at my computer. |
4aa2083
to
17f6449
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good !
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, thanks!
Thank you, @chuck! |
This PR