Fix: Use more appropriate assertions #37
Conversation
| $output = ob_get_clean(); | ||
|
|
||
| $this->assertEmpty($output); | ||
| $this->assertSame('', $output); |
There was a problem hiding this comment.
Probably better to compare with an empty string, as
$this->assertEmpty('[]');will pass.
There was a problem hiding this comment.
Could check strlen() also, but this is fine imo.
| $jsonEncoder = new Json(); | ||
| $encodedTrace = $jsonEncoder->encodeTraces([[$span]]); | ||
| $this->assertEquals($expectedPayload, $encodedTrace); | ||
| $this->assertJsonStringEqualsJsonString($expectedPayload, $encodedTrace); |
There was a problem hiding this comment.
This assertion will also give a more helpful error message when it fails.
tests/Unit/SpanContextTest.php
Outdated
| ]); | ||
|
|
||
| $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.
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.
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.
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.
chuck
left a comment
chuck
left a comment
There was a problem hiding this comment.
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. |
localheinz
force-pushed
the
fix/assertion
branch
from
4aa2083 to
17f6449
Compare
localheinz
changed the title
|
Thank you, @chuck! |
4 participants
This PR