While I was working on splitting fields into two patch, I found that the field tests were coming back a-okay, despite having not updated the tests for my changes.

Haven't looked into enough to see if it was the api functions returning incorrectly indicating there should be tests for that or those tests just need some updating.

To reproduce: rip out the insides of a hook_x_default_y in feature_tests, (like fields in d7, content in d6), run tests, watch them pass.

CommentFileSizeAuthor
#3 1402312_features_alter_tests_2.patch23.48 KBhefox
#1 1402312_features_alter_tests.patch14.85 KBhefox

Comments

hefox’s picture

hefox commented
Status: Active » Needs work
StatusFileSize
new 1402312_features_alter_tests.patch14.85 KB

Working on tests that detect more, including using alter hooks. This is a patch just to show someone.

Note includes patch in #1401686: Caching in features_get_default doesn't handle $alter argument

The tests are failing due to #766264: Alter hooks causing status to always be overridden

hefox’s picture

hefox commented

Updating patching.

new component hook hook_features_overridden($module) that if implemented, components can say whether they're features are overriden. This is needed for image -- features_get_default for image cannot determine the image styles correctly cause it doesn't have a proper alter hook, and it builds up the image style on load (adding stuff like module name) (like menu system does). This hook should also make features get component states faster, as the logic of checking the overridden faster is faster then rendering the default hook and checking that.

All tests are passing with that ^ hook in.

hefox’s picture

hefox commented
StatusFileSize
new 1402312_features_alter_tests_2.patch23.48 KB
Grayside’s picture

Grayside commented

Overall the need for a component to report in on whether it's in a shitty state seems shitty, but it makes sense to have that kind of flexibility to support alternate plugin engines. (ctools plugins vs image ad hoc). I imagine every Features integration will need it's own mechanism to explain it?

Will need to review again with the patch applied later, but this seems like a good direction.

Other possibility, drive a core patch since this is a vaguely significant change and only needed by Image. Could attempt to treat all instances like this as bugs.

Now, a few dreditor-powered nitpicks.

+++ b/features.module
@@ -889,3 +889,18 @@ function features_hook_info() {
+ * The a map between rebuild status and user friendly text.

Should be just 'a', no?

+++ b/tests/features.test
@@ -42,7 +42,10 @@ class FeaturesUserTestCase extends DrupalWebTestCase {
+    global $features_test_test_altering;

Huh?

+++ b/tests/features.test
@@ -54,39 +57,76 @@ class FeaturesUserTestCase extends DrupalWebTestCase {
+  ¶
...
+  ¶
...
+  ¶
...
+        $this->$callback('reset');

Whitespace

+++ b/tests/features.test
@@ -54,39 +57,76 @@ class FeaturesUserTestCase extends DrupalWebTestCase {
+   * Tests whether a component is currently in a set of possible states.

Confusing description. Probably should read "allowed states" and have a second text section explaining what its for.

hefox’s picture

hefox commented

To clarify, this is a suggestion of a patch then a real one -- it combines #1401686: Caching in features_get_default doesn't handle $alter argument and fix for #766264: Alter hooks causing status to always be overridden and this (the tests). I'll fix it up and separate it later (thus why needs work status instead of of needs review); my question/needs review part is whether testing like this is correct/desirable; I'm rather new to writing tests.

The global is being used cause need to turn on and off the alter hooks in features_test to test that the alters work without causing issues.