Fixed inconsistency in calculating date difference between platforms by morozov · Pull Request #3108 · doctrine/dbal

morozov · 2018-04-14T02:38:23Z

Original symptom: the test fails on SQLite depending on the current time and timezone:

↪ php -r "echo date('r'), PHP_EOL;"
Sat, 14 Apr 2018 18:26:33 -0700

↪ phpunit --filter testDateArithmetics tests/Doctrine/Tests/DBAL/Functional/DataAccessTest.phpPHPUnit 7.1.3 by Sebastian Bergmann and contributors.

Testing Doctrine\Tests\DBAL\Functional\DataAccessTest
F                                                                                   1 / 1 (100%)

Time: 84 ms, Memory: 6.00MB

There was 1 failure:

1) Doctrine\Tests\DBAL\Functional\DataAccessTest::testDateArithmetics
Date difference should be approx. -3024.9583333333 days.
Failed asserting that '-3026.0' matches expected -3024.9583333333335.

tests/Doctrine/Tests/DBAL/Functional/DataAccessTest.php:557

FAILURES!
Tests: 1, Assertions: 1, Failures: 1.

Reason:

The test relies upon date('Y-m-d') and AbstractPlatform:::getCurrentTimestampSQL() always producing dates in the same day. It's not true in case of SQLite whose CURRENT_TIMESTAMP is always in UTC or in general if the DB timezone is different from the PHPs.

Additional issue:

function diff(string $date1, string $date2) {
    $conn = \Doctrine\DBAL\DriverManager::getConnection([
        'driver' => 'pdo_sqlite',
        'memory' => true,
    ]);

    $platform = $conn->getDatabasePlatform();
    $sql = 'SELECT ' . $platform->getDateDiffExpression('?', '?');

    return $conn->executeQuery(
        $sql,
        [$date1, $date2]
    )->fetchColumn();
}

// two close enough dates on the same day
echo diff('2018-04-14 18:00:00', '2018-04-14 08:00:00'), PHP_EOL; // 0.0, correct

// two not close enough dates on the same day
echo diff('2018-04-14 18:00:00', '2018-04-14 06:00:00'), PHP_EOL; // 1.0, incorrect

// two close enough dates on subsequent days
echo diff('2018-04-15 02:00:00', '2018-04-14 22:00:00'), PHP_EOL; // 0.0, incorrect

Reason:

The Oracle and SQLite platforms, instead of comparing days of the dates, compare the datetimes and round the result. Instead, they should extract days from the datetimes and compare them.

…mySelectSQL() For testing purposes and not only, it may be needed to evaluate an arbitrary SQL expression (see doctrine#3013, doctrine#3108). Instead of doing `str_replace()` on an expected string, one could specify the expression explicitly.

morozov · 2018-04-15T03:57:38Z

tests/Doctrine/Tests/DBAL/Functional/DataAccessTest.php

        $row = array_change_key_case($row, CASE_LOWER);

-        $diff = (strtotime('2010-01-01') - strtotime(date('Y-m-d'))) / 3600 / 24;
-        self::assertEquals($diff, $row['diff'], "Date difference should be approx. ".$diff." days.", 1);


This "approx." and $round = 1 hide the existing issue. Basically, even if the expected and actual values are different by one, the test would still pass. The assertion is reworked in DateExpressionTest.

Majkl578

👍

Majkl578 · 2018-04-28T22:37:22Z

tests/Doctrine/Tests/DBAL/Functional/Platform/DateExpressionTest.php

+class DateExpressionTest extends DbalFunctionalTestCase
+{
+    /**
+     * @test


Imho we should keep it consistent and use testDifference or so.

Majkl578 · 2018-04-28T22:42:57Z

tests/Doctrine/Tests/DBAL/Functional/Platform/DateExpressionTest.php

+     * @test
+     * @dataProvider dateDiffProvider
+     */
+    public function diff(string $date1, string $date2, int $expected)


Majkl578 · 2018-04-28T22:43:12Z

tests/Doctrine/Tests/DBAL/Functional/Platform/DateExpressionTest.php

+        self::assertEquals($expected, $diff);
+    }
+
+    public static function dateDiffProvider()


: array / : iterable

It feels like a grey area — not that you're asking to add a return type but our current capabilities of specifying them. Adding array or iterable requires specifying a collection element type which is always a tuple which we don't have a type for. The closest compatible specification will be string[][]|int[][] which is re-e-eally not what this method is supposed to return.

Well we can disable specifying array/iterable/collection types in tests/, but return type should still be in place...

I think it's a good idea. The primary reason to require the specification is to see if the method is compatible with what the caller expects. But providers are not part of any interface, they are "magically" annotated methods which PHPUnit calls. So we shouldn't care that much.

Majkl578 · 2018-04-28T22:43:28Z

tests/Doctrine/Tests/DBAL/Functional/Platform/DateExpressionTest.php

+            $date2->modify('midnight')
+        )->days;
+
+        return [


Could use yields too. 😜

I don't know. It won't save any intermediate variable which would collect what's supposed to be returned, so I'd leave it as is.

i quite like it, it saves one nesting level and nested arrays of arrays (of arrays). But functionally it's irrelevant.

The SQLite and Oracle platforms produces a `ROUND()`'ed or `TRUNC()`'ated difference between the datetimes. However it should be the difference between dates.

…mySelectSQL() For testing purposes and not only, it may be needed to evaluate an arbitrary SQL expression (see doctrine#3013, doctrine#3108). Instead of doing `str_replace()` on an expected string, one could specify the expression explicitly.

morozov force-pushed the get-date-diff-expression branch 3 times, most recently from aa5e1c8 to deba5e7 Compare April 14, 2018 16:58

morozov mentioned this pull request Apr 14, 2018

Allow to specify arbitrary SQL expression in AbstractPlatform::getDummySelectSQL() #3109

Merged

morozov force-pushed the get-date-diff-expression branch 5 times, most recently from 509ab71 to 3752823 Compare April 15, 2018 03:49

morozov commented Apr 15, 2018

View reviewed changes

morozov requested a review from Majkl578 April 15, 2018 04:06

morozov added Failing Test Bug SQLite Oracle labels Apr 15, 2018

morozov assigned Ocramius Apr 15, 2018

Majkl578 reviewed Apr 28, 2018

View reviewed changes

morozov force-pushed the get-date-diff-expression branch from 3752823 to ede6aef Compare April 28, 2018 23:16

Fixed inconsistency in handling date differences between platforms

eb523ed

The SQLite and Oracle platforms produces a `ROUND()`'ed or `TRUNC()`'ated difference between the datetimes. However it should be the difference between dates.

morozov force-pushed the get-date-diff-expression branch from ede6aef to eb523ed Compare April 28, 2018 23:17

Majkl578 approved these changes Apr 29, 2018

View reviewed changes

Majkl578 requested a review from Ocramius April 29, 2018 20:36

morozov merged commit 755c99e into doctrine:master May 5, 2018

morozov deleted the get-date-diff-expression branch May 13, 2018 19:27

morozov added this to the 2.8.0 milestone Nov 2, 2019

github-actions bot locked as resolved and limited conversation to collaborators Aug 6, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fixed inconsistency in calculating date difference between platforms#3108

Fixed inconsistency in calculating date difference between platforms#3108
morozov merged 1 commit intodoctrine:masterfrom
morozov:get-date-diff-expression

morozov commented Apr 14, 2018 •

edited

Loading

Uh oh!

morozov Apr 15, 2018 •

edited

Loading

Uh oh!

Majkl578 left a comment

Uh oh!

Majkl578 Apr 28, 2018

Uh oh!

Majkl578 Apr 28, 2018

Uh oh!

Majkl578 Apr 28, 2018

Uh oh!

morozov Apr 28, 2018 •

edited

Loading

Uh oh!

Majkl578 Apr 28, 2018

Uh oh!

morozov Apr 28, 2018

Uh oh!

Majkl578 Apr 28, 2018

Uh oh!

morozov Apr 28, 2018

Uh oh!

Majkl578 Apr 28, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

morozov commented Apr 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

morozov Apr 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Majkl578 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

morozov Apr 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

morozov commented Apr 14, 2018 •

edited

Loading

morozov Apr 15, 2018 •

edited

Loading

morozov Apr 28, 2018 •

edited

Loading