Skip to content
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

[HttpFoundation] Call AssertEquals with proper parameters #33550

Open
wants to merge 1 commit into
base: 4.4
from

Conversation

@mmokhi
Copy link
Contributor

commented Sep 11, 2019

Since $response->getContent() returns string and our first parameter is already string as well, in some cases (with different precisions) it may "compare strings" as "strings" and this is not what the test wants.
By changing the first parameter to actual number we force AssertEquals to compare them numerically rather than literally by string content.

Q A
Branch? 3.4, 4.3, master
Bug fix? yes
New feature? no
Deprecations? no
Tickets Fix #...
License MIT
Doc PR symfony/symfony-docs#...

This is yet another catch of this type: #31612

Call AssertEquals with proper parameters
Since `$response->getContent()` returns string and our first parameter is already string as well, in some cases (with different precisions) it may "compare strings" as "strings" and this is not what the test wants.
By changing the first parameter to actual number we force `AssertEquals` to compare them numerically rather than literally by string content.

@nicolas-grekas nicolas-grekas modified the milestones: next, 3.4 Sep 11, 2019

@nicolas-grekas nicolas-grekas changed the title Call AssertEquals with proper parameters [HttpFoundation] Call AssertEquals with proper parameters Sep 11, 2019

@@ -43,7 +43,7 @@ public function testConstructorWithSimpleTypes()
$this->assertSame('0', $response->getContent());
$response = new JsonResponse(0.1);
$this->assertEquals('0.1', $response->getContent());
$this->assertEquals(0.1, $response->getContent());

This comment has been minimized.

Copy link
@stof

stof Sep 11, 2019

Member

this change is wrong. We are precisely asserting the string content of the response here.

This comment has been minimized.

Copy link
@mmokhi

mmokhi Sep 11, 2019

Author Contributor

But the string content can vary depending on the ini confs for floating point precision...
Here if the goal is to test the value being serialized correctly I think just evaluating the value should be the thing we want...

If we really care about 0.1 vs 0.10000000000000001 (I pasted the exact same value) we probably have to set some exact precision in the .ini file that PHPUnit is going to use...

One caveat of this test I could even catch, was 0.1 and 0.10 aren't also being treated the same...

Or it might be that I'm not correctly understanding the goal of this specific test though 🙂 🤔

This comment has been minimized.

Copy link
@stof

stof Sep 11, 2019

Member

well, a PHP float is not a proper json content. This test is about asserting the fact that JsonResponse properly encodes various types in JSON.

This comment has been minimized.

Copy link
@mmokhi

mmokhi Sep 11, 2019

Author Contributor

This test is about asserting the fact that JsonResponse properly encodes various types in JSON.

Exactly, this what I thought as well 🙂 👍
And that's why I proposed this change, because comparing string to string will have a lot of false-negatives (depending on how long the precision would be, 14 digits for Zend-engine default AFAIK).
But if we have one side of comparision as a normal number and the other side ( $response->getContent() ) as string, it'll force the Assert to convert both sides to the same type once (which will apply the local configs), and that way we'll no more have to worry about having a test .ini file and also we won't get this type of false-negatives 🙂

well, a PHP float is not a proper json content

I'm afraid this wasn't clear for me 😅 yes of course some floating-point 6bit data isn't what json sees as valid... but "0.100000001" is valid, no? 🤔

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.