With patch

Drupal test run
---------------

Tests to be run:
 - Locking framework tests (Drupal\system\Tests\Lock\LockFunctionalTest)
 - Locking framework unit tests (Drupal\system\Tests\Lock\LockUnitTest)

Test run started:
 Monday, January 21, 2013 - 21:38

Test summary
------------

Locking framework tests 18 passes, 0 fails, and 0 exceptions
Locking framework unit tests 9 passes, 0 fails, and 0 exceptions

Test run duration: 7 sec

real	0m7.640s
user	0m4.466s
sys	0m0.361s

Without patch

Drupal test run
---------------

Tests to be run:
 - Locking framework tests (Drupal\system\Tests\Lock\LockFunctionalTest)

Test run started:
 Monday, January 21, 2013 - 21:43

Test summary
------------

Locking framework tests 29 passes, 0 fails, and 0 exceptions

Test run duration: 17 sec

real	0m17.828s
user	0m12.636s
sys	0m0.829s

The two extra assertions are is that system_test module is enable for each of the test functions moved the unit test and it doesn't need to be.

CommentFileSizeAuthor
#6 4-6-interdiff.txt465 bytesalexpott
#6 1894960.drupal8.lock-unit-test.6.patch4.79 KBalexpott
#4 0-4-interdiff.txt3.32 KBalexpott
#4 1894960.drupal8.lock-unit-test.4.patch4.77 KBalexpott
drupal8.lock-unit-test.patch4.83 KBalexpott

Comments

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Active » Needs review

Go bot...

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Issue tags: +Test suite performance
dawehner’s picture

dawehner
Primary language German
commented

Aweseome, this seems to be nearly RTBC already.

+++ b/core/modules/system/lib/Drupal/system/Tests/Lock/LockUnitTest.phpundefined
@@ -0,0 +1,90 @@
+ * Contains Drupal\system\Tests\Lock\LockUnitTest.

Nitpick alarm!! Missing \

+++ b/core/modules/system/lib/Drupal/system/Tests/Lock/LockUnitTest.phpundefined
@@ -0,0 +1,90 @@
+    $backend = lock();
...
+    $success = $backend->acquire('lock_a');
...
+    $backend = lock();

I'm wondering whether this could should better use $this->container->get('lock');

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
StatusFileSize
new 1894960.drupal8.lock-unit-test.4.patch4.77 KB
new 0-4-interdiff.txt3.32 KB

Thanks for the review :)

Reading @crell's nice review http://drupal.org/node/1890784#comment-7011502 ... we should make unit test more unit-y by...

Or, really, this is in a test so you shouldn't be calling the container in the first place. Especially inside DUTB. Just make objects yourself and use 'em. That's how you keep unit tests unit-y.

The patch attached does that and fixes the nitpick too.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs review » Needs work 
+++ b/core/modules/system/lib/Drupal/system/Tests/Lock/LockUnitTest.phpundefined
@@ -0,0 +1,89 @@
+   * @var DatabaseLockBackend

This should be fully qualified as well now. Looks great otherwise.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Needs work » Needs review
StatusFileSize
new 1894960.drupal8.lock-unit-test.6.patch4.79 KB
new 4-6-interdiff.txt465 bytes

Here we go...

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs review » Reviewed & tested by the community

Great, looks ready to me.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 1894960.drupal8.lock-unit-test.6.patch, failed testing.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

Upgrade path tests fail... ho hum :(

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Needs work » Needs review

#6: 1894960.drupal8.lock-unit-test.6.patch queued for re-testing.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

Back to RTBC.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs review » Reviewed & tested by the community
catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Yep. Committed/pushed to 8.x.

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