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

LegacyDBTest testDecimal is actually failing on sqlite database #34880

Closed
phil-davis opened this issue Mar 23, 2019 · 3 comments
Closed

LegacyDBTest testDecimal is actually failing on sqlite database #34880

phil-davis opened this issue Mar 23, 2019 · 3 comments

Comments

@phil-davis
Copy link
Contributor

Steps to reproduce

  1. upgrade PHPUnit to PHPUnit 7 - see PR [WIP] Bump phpunit 7 #34877
  2. make test-php-unit TEST_DATABASE=sqlite TEST_PHP_SUITE=tests/lib/DB/LegacyDBTest.php

Expected behaviour

Test passes

Actual behaviour

PHPUNIT="/home/phil/git/owncloud/core/lib/composer/phpunit/phpunit/phpunit" build/autotest.sh sqlite tests/lib/DB/LegacyDBTest.php
Using PHP executable /usr/bin/php
Parsing all files in lib/public for the presence of @since or @deprecated on each method...

Using database oc_autotest
Setup environment for sqlite testing on local storage ...
Installing ....
creating sqlite db
ownCloud was successfully installed
Testing with sqlite ...
No coverage
/home/phil/git/owncloud/core/lib/composer/phpunit/phpunit/phpunit --configuration phpunit-autotest.xml --log-junit autotest-results-sqlite.xml ../tests/lib/DB/LegacyDBTest.php 
PHPUnit 7.5.7 by Sebastian Bergmann and contributors.

Runtime:       PHP 7.2.16-1+ubuntu18.04.1+deb.sury.org+1 with Xdebug 2.7.0
Configuration: /home/phil/git/owncloud/core/tests/phpunit-autotest.xml

............F............                                         25 / 25 (100%)

Time: 534 ms, Memory: 20.00 MB

There was 1 failure:

1) Test\DB\LegacyDBTest::testDecimal with data set #1 ('1234567890', '1234567890.00')
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'1234567890.00'
+'1234567890'

/home/phil/git/owncloud/core/tests/lib/DB/LegacyDBTest.php:295

FAILURES!
Tests: 25, Assertions: 81, Failures: 1.
Makefile:176: recipe for target 'test-php-unit' failed
make: *** [test-php-unit] Error 1

Server configuration

git core master clone and run unit tests

@phil-davis
Copy link
Contributor Author

phil-davis commented Mar 23, 2019

In PHPUnit v5 and v6 assertEquals() was doing a more "loose" equals comparison. Strings that "looked like numbers" were being compared in terms of their numeric value, so "123.00" and "123" were considered equal - which they are as numbers.

Since https://github.com/sebastianbergmann/comparator/pull/58/files , if both things to be compared are strings, a strict string comparison is done. Strings that are different but "look like the same number" are now actually seen as different and will fail assertEquals()

Other related discussion of this PHPUnit7 behavior change (improvement IMHO) sebastianbergmann/phpunit#3185

This has revealed this difference in database behavior, which the unit test did not previously "notice".

In mySQL you put "1234567890" into a field of decimal(12,2) and when you query the database you get back "1234567890.00" - which is the test expectation.

In sqlite you put "1234567890" into a field of decimal(12,2) and when you query the database you get back "1234567890" - it does not put the 2 fractional digits or decimal point when the number is an exact integer.

Similar happens in sqlite if you put "1234567890.00" into a field of decimal(12,2) - when you query the database you get back "1234567890"

@phil-davis
Copy link
Contributor Author

Do we care about this difference?

If yes, then it needs to be "fixed" in some database abstraction layer to "hide" the underlying database difference.

If no, then we can adjust the unit test to not be so strict about the exact output that it expects.

@phil-davis phil-davis mentioned this issue Mar 23, 2019
11 tasks
@stale
Copy link

stale bot commented Sep 20, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 10 days if no further activity occurs. Thank you for your contributions.

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

No branches or pull requests

2 participants