This patch moves the timer_* functions to a utility class so it can be unit tested outside of drupal, and converts the existing test to phpunit.

The original test was waiting way too long (4 seconds) and only asserting greater than, so the accuracy of the timers wasn't well tested. The new assertions test both greater and less than, and reduces the sleep time to 5ms.

CommentFileSizeAuthor
#11 drupal-1935970-11.patch9.01 KBdawehner
#9 drupal-1935970-9.patch9 KBdawehner
#9 interdiff.txt3.67 KBdawehner
#8 inter.diff982 bytesRobLoach
#8 1935970.patch9.08 KBRobLoach
#5 unit-test-timers-5.patch8.98 KBmsonnabaum
#4 unit-test-timers-4.patch8.95 KBmsonnabaum
unit-test-timers.patch8.27 KBmsonnabaum
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Status: Needs review » Needs work

The last submitted patch, unit-test-timers.patch, failed testing.

msonnabaum’s picture

msonnabaum CreditAttribution: msonnabaum commented
Status: Needs work » Needs review

unit-test-timers.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, unit-test-timers.patch, failed testing.

msonnabaum’s picture

msonnabaum CreditAttribution: msonnabaum commented
Status: Needs work » Needs review
FileSize
unit-test-timers-4.patch8.95 KB

Hmm, looks like we don't register the autoloader early enough. This patch moves it farther up.

msonnabaum’s picture

msonnabaum CreditAttribution: msonnabaum commented
FileSize
unit-test-timers-5.patch8.98 KB

Last one prevents configuring the autoloader, so moving it back down a bit.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented

Just referencing a similar/reated issue: #1893306: Put the timers_() functions into a seperate component

rbayliss’s picture

rbayliss CreditAttribution: rbayliss commented

Not to bikeshed, but would this be a good time to replace our current timer implementation with the new Symfony Stopwatch component?

RobLoach’s picture

RobLoach
he/him
CreditAttribution: RobLoach commented
FileSize
1935970.patch9.08 KB
inter.diff982 bytes 
$ cd core
$ phpunit
OK (17 tests, 127 assertions)
$ cd ..
$ wget http://drupal.org/files/unit-test-timers-5.patch
$ git apply unit-test-timers-5.patch
$ cd core
$ phpunit
OK (18 tests, 139 assertions)

Updated some of the docblocks.

Not to bikeshed, but would this be a good time to replace our current timer implementation with the new Symfony Stopwatch component?

Let's make an issue for that: #1936640: Replace the Timer component with Symfony Stopwatch

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
FileSize
interdiff.txt3.67 KB
drupal-1935970-9.patch9 KB 
+++ b/core/includes/bootstrap.incundefined
@@ -2270,16 +2227,18 @@ function _drupal_exception_handler($exception) {
+  timer_start('page');

Let's use the class already.

+++ b/core/includes/bootstrap.incundefined
@@ -2270,16 +2227,18 @@ function _drupal_exception_handler($exception) {
   // Make sure we are using the test database prefix in child Drupal sites.
   _drupal_initialize_db_test_prefix();

It seems to be that we want to setup the database prefixes as early as possible, so let's keep this before loading the classes.

Just some small doc fixes included.

msonnabaum’s picture

msonnabaum CreditAttribution: msonnabaum commented

Any talk about switching timers is very offtopic, let's keep it in the other issue.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
FileSize
drupal-1935970-11.patch9.01 KB

Just another rerole.

RobLoach’s picture

RobLoach
he/him
CreditAttribution: RobLoach commented
Status: Needs review » Reviewed & tested by the community
RobLoach’s picture

RobLoach
he/him
CreditAttribution: RobLoach commented
Issue tags: +PHPUnit Blocker

Status: Reviewed & tested by the community » Needs work
Issue tags: -PHPUnit Blocker

The last submitted patch, drupal-1935970-11.patch, failed testing.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Status: Needs work » Needs review
Issue tags: +PHPUnit Blocker

#11: drupal-1935970-11.patch queued for re-testing.

RobLoach’s picture

RobLoach
he/him
CreditAttribution: RobLoach commented
Status: Needs review » Reviewed & tested by the community
xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented

#11: drupal-1935970-11.patch queued for re-testing.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Reviewed & tested by the community » Fixed
Issue tags: -PHPUnit Blocker +Needs change record

Committed 291dbd5 and pushed to 8.x. Thanks!

jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran commented
Title: Convert timer_* to a utility class and convert tests to phpunit » Change notice: Convert timer_* to a utility class and convert tests to phpunit
Priority: Normal » Critical
Status: Fixed » Active
Issue tags: -Needs change record +PHPUnit Blocker

For change notice.

ParisLiakos’s picture

ParisLiakos CreditAttribution: ParisLiakos commented
Title: Change notice: Convert timer_* to a utility class and convert tests to phpunit » Change notice: Convert timer_* to a utility class and convert tests to phpunit

Awesomeness!
Also should we create folllowups to get rid of procedural wrappers?

jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran commented
Title: Change notice: Convert timer_* to a utility class and convert tests to phpunit » Change notice: Convert timer_* to a utility class and convert tests to phpunit
Issue tags: +Needs change record

re-tagging.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented

Looks like this introduced a random test failure: #1960032: Random failure in TimerUnitTest

ParisLiakos’s picture

ParisLiakos CreditAttribution: ParisLiakos commented
Title: Change notice: Convert timer_* to a utility class and convert tests to phpunit » Convert timer_* to a utility class and convert tests to phpunit
Priority: Critical » Normal
Status: Active » Fixed
Issue tags: -Needs change record

Change notice
http://drupal.org/node/1969828

Automatically closed -- issue fixed for 2 weeks with no activity.

jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
CreditAttribution: jhedstrom commented
Issue summary: View changes
Parent issue: » #1938068: Convert UnitTestBase to PHPUnit