Objective

  1. #2188661: Extension System, Part II: ExtensionDiscovery changes the extension discovery process to ignore all test extensions at regular runtime.
  2. Sometimes it is useful to enable a test module for manual (test) debugging purposes.

Proposed solution

  1. Add a settings.php variable to allow to discover test extensions at regular runtime.
CommentFileSizeAuthor
#21 2198713-21-discover-test-extensions.patch844 byteststoeckler
#7 2198713-7-test-extensions.patch1.7 KBtstoeckler
#5 2198713-5-test-extensions.patch2.35 KBtstoeckler
#2 2198713-1-test-extensions.patch2.32 KBtstoeckler
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Component: base system » extension system
tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler commented
Status: Postponed » Needs review
FileSize
2198713-1-test-extensions.patch2.32 KB

Here we go. Tested this locally.

This might not qualify for "major" in the Drupal issue queue sense, but in general I consider this a pretty major feature request, and is arguably a regression.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented 
  1. +++ b/core/lib/Drupal/Core/Extension/ExtensionDiscovery.php
@@ -140,8 +142,10 @@ public function scan($type, $include_tests = NULL) {
+      $include_tests = drupal_valid_test_ua() || $settings->get('discover_test_extensions');

    Can we retain the cast to (bool)? drupal_valid_test_ua() returns a string.

    Can we rename the setting name to be at least in the namespace of the core service? E.g.,

    "extension_discovery_scan_tests"

  2. +++ b/sites/default/default.settings.php
@@ -512,6 +512,14 @@
 /**
+ * Include test directories when discovering extensions.

    Not sure whether we really need a default comment/template in settings.php... (this file is painfully large already)

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler commented

Re #3:

1.
a The || casts to bool already. I verified this locally, as well. Try php -r 'var_export("string" || FALSE); print "\n";' in your command line.

b Sure, will do

2. As far as I'm aware all settings are currently documented in settings.php so while I agree with you that it's unwieldy currently, I'd rather not break with that rule for consistency.

Also: I thought about adding this as a UI setting somewhere, as certain test modules, actually make sense to use in the UI. For example, the infamous design_test module. Don't know how people feel about that, though.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler commented
FileSize
2198713-5-test-extensions.patch2.35 KB

Here we go. Sorry, forgot to make an interdiff and couldn't be bothered to retroactively make one. I also slightly updated the settings.php comment.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented

Regarding 2), there are actually a lot of settings that are not mentioned in settings.php: #2160705: settings.php setting names are inconsistent (and undocumented)

I won't fight for removing the docs from settings.php here, I just don't think that this is a setting that regular Drupal users should have to bother with... everything we put into settings.php will at least have to be read (or skipped over) by every human. ;)

What do others think?

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler commented
FileSize
2198713-7-test-extensions.patch1.7 KB

I wasn't aware that that is the case. In that case I completely agree. Here it is without the default.settings.php hunk.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Status: Needs review » Reviewed & tested by the community

Thanks!

btw, I wasn't really aware of #4.1 — somehow, that never occurred to me; I feel dumb now.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler commented

Aww, that's no reason to feel dumb! :-)

I had just assumed that it worked that way, I had to try it out as well to be sure. Now we're both smarter, so teamwork++ :-)

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented

Closely related: #2209337: Remove "hidden" property from test extensions

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Status: Reviewed & tested by the community » Needs review

Hm. Really not sure about this. "hidden: true" in a .info file is super easy to manually switch to "false" in the case you want to enable a test module (I didn't realize this capability was effectively removed in an earlier patch; that sounds like a bad idea, though it's good for performance, I suppose). Somehow learning about a totally undocumented settings.php tweak that applies to *all* test modules (thus increasing the performance cost of ExtensionDiscovery::scan() across the board) doesn't sound like a good idea to me. I agree with not exposing it in settings.php because it's just noise for 99.99999% of people. But OTOH in its current state, the *only* way you'd discover this variable exists is via a very angry debugging session after minutes/hours of face smashing trying to figure out why the *&@! ExtensionDiscovery::scan() is not picking up your module like it's "supposed" to.

If we do do this, it seems like we need to expose a UI for it under admin/config/devel/testing or whatever. I would lean more toward restoring the capability for hidden true/false to work, though.

EDIT to fix the opposite of what I mean in the first few sentences. :P

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented

I would lean more toward restoring the capability for hidden true/false to work, though.

I was realllly confused when uncommenting hidden: true. We had no change notices to alert devs of this.
I agree we should revert that.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented

In case "revert that" refers to discovering test extensions at regular runtime:

Including test extension at regular runtime means 4x more results and a massive amount of additional directories that are scanned.

We're talking about hundreds of more results, and thousands of subdirectories that are additionally scanned. Unless your server happens to run on SSD, excluding them causes a critical difference in performance.

Therefore, including test extensions at regular runtime makes absolutely no sense. Doing so would seriously harm performance for no gain at all. Given the results and numbers I'm looking at on my local box, removing the exclusion of test extensions at regular time simply won't fix.

Now. Even though I'm most certainly one of the last remaining "legacy" developers who are hacking on core without an IDE:

The use-case of having to enable a test extension for advanced test debugging did not occur to me in the past months. I certainly had that use-case in the past, but not anymore today.

I want to believe that this is the positive result of transitioning more and more tests to DUTB — i.e., at least for me, the use-case of manually enabling a test module was almost always preceded by a malfunctioning web test that took ages to run, so instead of running the test, I wanted to manually debug the situation, in order to figure out the final fix for a test.

But as far as I learned, that use-case is completely obsolete and pretty much an alien to IDE developers today. Instead of doing that, you simply run tests with enabled debugger breakpoints instead — which yields much more comprehensive data to begin with.

Consequently:

  1. "Enabling a test module" is an extreme edge-case/use-case that only a handful of people (developers) will ever have.

  2. Making it easy to enable a test module for advanced debugging would only encourage bad practices to begin with:

    If you can only reproduce a bug by manually enabling a test module, then that is a clear sign that code logic is going wrong in lower layers.

    → Add some unit tests instead.

  3. Allowing to discover/enable test modules at regular runtime therefore is a highly dubious goal in the first place.

    (I only created this issue due to questions raised by others. The more I think about it, the less I support it.)

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented

One question would be whether or not there's a way in all of this extension hocus pocus to support scanning test directories when a module is enabled by the command line. Then "drush en foo_test" would still work, to meet the developer use case, but every other page request doesn't take the unnecessary hit.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented

Hm. Actually it occurs to me that a theoretical "enable hidden modules" UI in Devel or something might want this capability, too.

Can we simply move the "scan_tests" variable to a parameter of scan() that defaults to FALSE?

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented

Can we simply move the "scan_tests" variable to a parameter of scan() that defaults to FALSE?

That is exactly what this patch here is doing?

If we really want to further clutter settings.php with an additional setting for this, then #5 is the patch to commit. #7 is the same as #5, just without the default.settings.php addition.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler commented
Status: Needs review » Reviewed & tested by the community

@webchick: So if we move this to config, instead of $settings would you be more comfortable with it? I'd really like to move forward with this.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler commented
Status: Reviewed & tested by the community » Needs review

Oops, the status change was not intentional.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Related issues: +#2226761: Change all default settings and config to fast/safe production values

Not sure whether it is really 100% related (because I would not have a use-case for having test modules in a "dev" environment), but perhaps the existence of this issue just simply turns that the idea over there into bitwise-OR'ed flags instead...?

// Disable all production caching + find test extensions.
$settings['env'] = ENV_DEV | ENV_TEST;
tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler commented

Bump.

This is still really annoying. Can we get some progress on this, please? I'd be happy to work on this some more, but the latest feedback was not really actionable. Or maybe I misunderstood it. Really don't know how to bring this forward, though, right now... :-/

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler commented
FileSize
2198713-21-discover-test-extensions.patch844 bytes

Just realized this needed a re-roll.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Status: Needs review » Reviewed & tested by the community

Still makes sense to me — especially when combined with #2209337: Remove "hidden" property from test extensions

catch’s picture

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

Committed/pushed to 8.x, thanks!

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler commented
Status: Fixed » Reviewed & tested by the community

Just wanted to use this locally and realized this was never actually pushed. At least the code is unchanged and git log | grep 2198713 doesn't yield anything.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler commented
Status: Reviewed & tested by the community » Fixed

Sorry, I'm a complete and utter idiot.

Mixologic’s picture

Mixologic
He/Him/His
Primary language English
Location Portland, OR
CreditAttribution: Mixologic commented
Issue tags: -Extension system +Needs change record

Perhaps this needs a change record since no functionality was removed, yet DX rage exists because of its discoverability https://drupal.org/node/2279279

Status: Fixed » Closed (fixed)

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

mikey_p’s picture

mikey_p CreditAttribution: mikey_p commented

This definitely needs a CR since I spent a few minutes trying to Google an answer for this and found nothing. Luckily I caught larowlan in IRC. This is the type of thing that we should include in a developer.local_settings.php or something like that.

mikey_p’s picture

mikey_p CreditAttribution: mikey_p commented

Just noticed that this is included in sites/example.settings.local.php. That is a somewhat poor location, since that file seems to focus mainly on development only settings, like the development.services.yml, logging, null cache, rebuild access etc.

chx’s picture

chx CreditAttribution: chx commented
Issue tags: -Needs change record

And the change record is now (!) at https://www.drupal.org/node/2642834 . It was not filed by those who broke this. That's not the Drupal 8 way. No.