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

[unit tests] 1 second less #11888

Merged
merged 2 commits into from
Sep 3, 2016
Merged

Conversation

andrepereiradasilva
Copy link
Contributor

@andrepereiradasilva andrepereiradasilva commented Sep 2, 2016

Summary of Changes

Reduce unit tests time a little further, but removing to 1 seconds sleep in JFactoryTest::testGetDateNow (one line changed).

Testing Instructions

Unit tested passed.
Code review

Documentation Changes Required

None.

@csthomas
Copy link
Contributor

csthomas commented Sep 2, 2016

Code review failed.

Datetime precision is 1 second.

test at console:
$a = new Datetime();usleep(1);$b = new Datetime();
var_dump($a == $b); // bool(true)

and:
$a = new Datetime();sleep(1);$b = new Datetime();
var_dump($a == $b); // bool(false)

@andrepereiradasilva
Copy link
Contributor Author

andrepereiradasilva commented Sep 2, 2016

how do you test this at console? what's the command? i only tested on online travis.

Also i think DateTime supports microseconds since php 5.2.2 ->format('u')

@csthomas
Copy link
Contributor

csthomas commented Sep 2, 2016

On local computer type command:
php -a

You should see:

$ php -a
Interactive mode enabled

php >

@csthomas
Copy link
Contributor

csthomas commented Sep 2, 2016

@andrepereiradasilva
Copy link
Contributor Author

andrepereiradasilva commented Sep 2, 2016

I tought JDate was extended to have microseconds too
So 'now' for Joomla can be 0.99 seconds ago 😉

We could do something like this here https://github.com/joomla/joomla-cms/blob/staging/libraries/joomla/date/date.php#L107 for joomla to have the correct 'now' up to microsecond but i really don't know the impacts of this.

if ($date === 'now')
{
    $now   = microtime(true);
    $micro = sprintf("%06d", ($now - floor($now)) * 1000000);
    $date  = date('Y-m-d H:i:s.' . $micro, $now);
}

But, for now the 'now' is in seconds.

i will change this PR to 1 second them.

@mbabker
Copy link
Contributor

mbabker commented Sep 2, 2016

Since JDate is extending PHP's native DateTime, I wouldn't do too much with its internal data structure, especially if it causes weird side effects or breakage of features using the native class.

@andrepereiradasilva
Copy link
Contributor Author

Does adding microseconds when constructing a DateTime causes weird side effects or breakage of features using the native class ?

@mbabker
Copy link
Contributor

mbabker commented Sep 2, 2016

I don't know, I don't generally work with microseconds when using time based data (at least I haven't had a use case for it in 5+ years).

@andrepereiradasilva
Copy link
Contributor Author

i actually use microseconds a lot for performance tests.

@mbabker
Copy link
Contributor

mbabker commented Sep 2, 2016

Quick glance through the PHP docs says yes it is supported (use u in your format strings when formatting DateTime objects). So I guess it's fine.

@andrepereiradasilva
Copy link
Contributor Author

Patched microseconds here #11890

@andrepereiradasilva andrepereiradasilva changed the title [unit tests] 2 seconds less [unit tests] 1 second less Sep 2, 2016
@csthomas
Copy link
Contributor

csthomas commented Sep 2, 2016

I have tested this item ✅ successfully on

Code review.
1 second less is OK.


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/11888.

@wilsonge wilsonge merged commit 8276f07 into joomla:staging Sep 3, 2016
@wilsonge wilsonge added this to the Joomla 3.6.3 milestone Sep 3, 2016
@andrepereiradasilva andrepereiradasilva deleted the patch-21 branch September 3, 2016 11:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants