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

Fix date parsing in tests when TZ is changed to non-UTC during tests #16809

Merged
merged 1 commit into from Jan 9, 2020

Conversation

@mvorisek
Copy link
Contributor

mvorisek commented Dec 14, 2019

Questions Answers
Branch? develop
Description? Tests are failing after midnight.
Type? bug fix
Category? TE
BC breaks? no
Deprecations? no
Fixed ticket? no
How to test? Fork repo, setup Travis and push some commit after CEST midnight.

For some reasons, when I var_dumped the datetime used in the Validate::isBirthDate in the related ValidateCoreTest::testIsBirthDate test, the TZ was US/Eastern. This fixes it, the test data are generated in UTC.


This change is Reviewable

@mvorisek mvorisek requested a review from PrestaShop/prestashop-core-developers as a code owner Dec 14, 2019
@Progi1984

This comment has been minimized.

Copy link
Contributor

Progi1984 commented Dec 16, 2019

@mvorisek Thank your for your contribution 👐

Fixing a bug in a test is not a solution. A better solution could be to configure Travis to define the default timezone for PHP.

@mvorisek

This comment has been minimized.

Copy link
Contributor Author

mvorisek commented Dec 16, 2019

@Progi1984 In Travis / PHP unit the TZ is already reset. But somewhere in the PHP unit tests the TZ is set to the mentioned US/Eastern.

This is how I fixed the test locally. If all PHP unit tests should be tested strictly isolated, then the test changing the TZ needs to be isolated and fixed like this one and this reverted.

@matks

This comment has been minimized.

Copy link
Contributor

matks commented Dec 20, 2019

@mvorisek Thank your for your contribution 👐

Fixing a bug in a test is not a solution. A better solution could be to configure Travis to define the default timezone for PHP.

But tests must run in any environment, not only Travis. So we need to either setup such settings ourselves (like this PR does) or provide guidance for people for the right environment for these tests 🤔

@matks

This comment has been minimized.

Copy link
Contributor

matks commented Dec 20, 2019

What about we define globally a timezone ? Like in phpunit bootstrap file ? This fixes it once and for all and we do not care about "setting back the timezone" after the test was run ?

@mvorisek

This comment has been minimized.

Copy link
Contributor Author

mvorisek commented Dec 20, 2019

@matks

What about we define globally a timezone ? Like in phpunit bootstrap file ? This fixes it once and for all and we do not care about "setting back the timezone" after the test was run ?

There are two things:

  1. TZ set/reset to UTC, currently TZ is set in https://github.com/PrestaShop/PrestaShop/blob/develop/composer.json . If you run tests thru composer command, the TZ is set corrently everywhere.
  2. TZ changed during tests. It seems there is only one issue and this PR solves it. Or, as this issue is related with birth date, we can update the tests to not test values +- 24 hours around current time. Birth date in context of people and PrestaShop this makes sense too - is it better to solve this issue this way?
@mvorisek mvorisek changed the title Fix date parsing in tests when TZ is changed to non-UTC during tests WIP: Fix date parsing in tests when TZ is changed to non-UTC during tests Dec 24, 2019
@mvorisek

This comment has been minimized.

Copy link
Contributor Author

mvorisek commented Jan 1, 2020

@Progi1984, @matks I have further analysed this issue as it breaks unit tests after UTC midnight on every test enviroment (incl. TravisCI, see https://travis-ci.org/mvorisek/PrestaShop/jobs/631449993#L683 , test run on the latest develop)

I have run all unit tests with xdebug tracing enabled and the TZ is set to US/Eastern only at this line:

date_default_timezone_set() ./config/config.inc.php:151

The issue is that phpunit data providers are run just after the phpunit bootstrap. The config file above, even it is a config file, was included at time 53.8167 sec and 70156528 lines after the phpunit has started and it requires DB. I do not think that this file can be moved into the bootstrap.php file easily.

As of now this is the only test failing due too data/dates without TZ from phpunit data provider. Can this PR be merged or can you advise another fix?

@mvorisek mvorisek changed the title WIP: Fix date parsing in tests when TZ is changed to non-UTC during tests Fix date parsing in tests when TZ is changed to non-UTC during tests Jan 2, 2020
@mvorisek

This comment has been minimized.

Copy link
Contributor Author

mvorisek commented Jan 9, 2020

Any feedback?

@PierreRambaud

This comment has been minimized.

Copy link
Contributor

PierreRambaud commented Jan 9, 2020

Why not using this in the travis.yml file?

before_install:
- export TZ=UTC
@matks

This comment has been minimized.

Copy link
Contributor

matks commented Jan 9, 2020

Why not using this in the travis.yml file?

before_install:
- export TZ=UTC

It means we only guarantee the tests work in Travis environment, not in another testing environment such as the developer computer 😥

@PierreRambaud

This comment has been minimized.

Copy link
Contributor

PierreRambaud commented Jan 9, 2020

@matks This is because php, server and database are not configure properly, it's depending on the system and not the application 🤔

@matks

This comment has been minimized.

Copy link
Contributor

matks commented Jan 9, 2020

@matks This is because php, server and database are not configure properly, it's depending on the system and not the application 🤔

OK, but then it means "prestashop unit tests must be run only in an environment with UTC timezone". So either the developer makes the right settings for his local environment or we do this:

What about we define globally a timezone ? Like in phpunit bootstrap file ? This fixes it once and for all and we do not care about "setting back the timezone" after the test was run ?

@PierreRambaud

This comment has been minimized.

Copy link
Contributor

PierreRambaud commented Jan 9, 2020

@matks This is because php, server and database are not configure properly, it's depending on the system and not the application thinking

OK, but then it means "prestashop unit tests must be run only in an environment with UTC timezone". So either the developer makes the right settings for his local environment or we do this:

What about we define globally a timezone ? Like in phpunit bootstrap file ? This fixes it once and for all and we do not care about "setting back the timezone" after the test was run ?

I was more thinking about mismatch configuration, many people are talking about timezone problem in Travis, maybe we can use both, the phpunit.xml with timezone in UTC, and the global travis UTC timezone, wdyt?

@matks

This comment has been minimized.

Copy link
Contributor

matks commented Jan 9, 2020

@matks This is because php, server and database are not configure properly, it's depending on the system and not the application thinking

OK, but then it means "prestashop unit tests must be run only in an environment with UTC timezone". So either the developer makes the right settings for his local environment or we do this:

What about we define globally a timezone ? Like in phpunit bootstrap file ? This fixes it once and for all and we do not care about "setting back the timezone" after the test was run ?

I was more thinking about mismatch configuration, many people are talking about timezone problem in Travis, maybe we can use both, the phpunit.xml with timezone in UTC, and the global travis UTC timezone, wdyt?

I like it 👍 it would be a clean solution

@mvorisek

This comment has been minimized.

Copy link
Contributor Author

mvorisek commented Jan 9, 2020

No.

TZ for tests is already set to UTC in:

"@php -d date.timezone=UTC ./vendor/bin/phpunit -c tests/Unit/phpunit.xml"

but later (re)set in the tests itself, more specifically in a config file, which is included during the tests:

@date_default_timezone_set(Configuration::get('PS_TIMEZONE'));

So the enviroment settings are irrelevant and this issue is presented also in current TravisCI - see https://travis-ci.org/mvorisek/PrestaShop/jobs/631449993#L683

Can this PR be merged? Or is it better to not use isBirthDateProvider as a data provider at all (the only data provider which is affected by this issue)?

Copy link
Contributor

PierreRambaud left a comment

My bad, your change is totally legit 👍

@matks

This comment has been minimized.

Copy link
Contributor

matks commented Jan 9, 2020

Since @PierreRambaud is 👍 LGTM

@matks

This comment has been minimized.

Copy link
Contributor

matks commented Jan 9, 2020

Thank you @mvorisek

@matks matks merged commit f8a26e1 into PrestaShop:develop Jan 9, 2020
2 checks passed
2 checks passed
PrettyCI Code formatting
Details
Travis CI - Pull Request Build Passed
Details
@matks matks added this to the 1.7.7.0 milestone Jan 9, 2020
@mvorisek mvorisek deleted the mvorisek:gf_fix_utc_tz_in_tests branch Jan 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.