Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Status: Needs review » Needs work

The last submitted patch, 1122224-test-libraries-caching-2.patch, failed testing.

tstoeckler’s picture

Status: Needs work » Needs review
FileSize
968 bytes

Let's try this one.

sun’s picture

Hm, not sure whether this test makes sense. The following would make more sense to me, but I'm not sure whether it is possible to test, and whether it's worth the effort:

libraries_load('example');
$this->assertCacheMiss();
libraries_load('example');
$this->assertCacheHit();

Otherwise, we're merely testing core's Cache API and not really our own caching mechanism.

But I somewhat doubt that these (pseudo) assertCacheMiss() and assertCacheHit() assertions are possible without introducing a really large chunk of testing code that might be a hack even.

Though, perhaps set a variable and just simply conditionally output a message in hook_libraries_info() of the testing module -- which should then only be triggered on a cache miss, if I get this right?

tstoeckler’s picture

If we do a drupal_set_message() libraries_test_libraries_info() that will effect all other tests. Otherwise we would have to make a new libraries_cache_test.module, just for that purpose.
What would also work, and be simpler IMO, is putting a drupal_set_message() in one of the callbacks (except the 'load' group, which gets called every time) and then assert on that message.
That should be doable, I think.

sun’s picture

Putting it into one of the cached callbacks sounds good. By wrapping it into a condition if (variable_get('libraries_test_cache', FALSE), it will only be output and affect this test.

tstoeckler’s picture

Ahh, that's what you meant with the variable. Yes, that absolutely makes sense.

sun’s picture

tstoeckler’s picture

Status: Needs review » Needs work

Needs work.
(And bumping. (For myself to see this tomorrow or so and pick it up.))

tstoeckler’s picture

Status: Needs work » Needs review
FileSize
4.23 KB

Here we go. I did a minimal cleanup in libraries_test_menu(), hope that doesn't put you off.

The tests pass, so the caching does work as intended.

tstoeckler’s picture

Status: Needs review » Reviewed & tested by the community

Just looked at this again, and couldn't find anything wrong with it. Since it's just tests for existing functionality, I'll probably commit this soon-ish.

sun’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/tests/libraries_test.module
@@ -414,34 +421,34 @@ function _libraries_test_callback(&$library, $version, $variant, $group) {
+  foreach ($items as &$item) {
+    $item += array(
+      'page callback' => '_libraries_test_load',
+      'access callback' => TRUE,
+    );
+  }

Let's define

$base = array(...);

and + that into the others:

$items[...] = $base + array(...);

Aside from that, this looks good to me.

tstoeckler’s picture

Status: Needs work » Needs review
FileSize
4.41 KB

Sure thing.
If this comes back green, I'll commit it.

tstoeckler’s picture

Status: Needs review » Fixed

Committed to 7.x-2.x.
Thanks for the quick feedback!
http://drupal.org/commitlog/commit/10030/e2472883c6dce4c75b66f41c08fc1db...

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