Follow-up from #1563620: All unit tests blow up with a fatal error.

<catch> berdir: how come we're not back in the parent environment by that point though?
<catch> I'd think $test->run(); should include messing with the db prefix both ways.
<berdir> catch: ah, one sec, this is actually something different and ok, yes.
<berdir> catch: well, it's easy to find out, I'll comment it out and see what happens ;)
<berdir> catch: I haven't touched that part myself yet
<berdir> catch: hah. It's simpletest's own verbose debug stuff that blows up, among other things
<berdir> catch: actually, that one is just because TableSortTest thinks it's necessary to do $this->verbose() on all kinds of stuff. should be cleaned up, but not there
<berdir> catch: yep, it's just that. for the current tests
<berdir> catch: we *could* remove it, fix TableSortTest and do something with verbose() in UnitTestBase
<berdir> catch: either forward to debug() or throw a not supported exception
<catch> berdir: that sounds much better tbh. This issue or a follow-up?
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

catch’s picture

So we should be able to revert the following I think, if we can genuinely ensure that no module_*() functions are called by simpletest itself during unit test runs, and no theme() functions either.

   if (!isset($drupal_static_fast)) {
-    $drupal_static_fast['list'] = &drupal_static(__FUNCTION__ . ':list', array());
+    $drupal_static_fast['list'] = &drupal_static(__FUNCTION__ . ':list');
     $drupal_static_fast['sorted_list'] = &drupal_static(__FUNCTION__ . ':sorted_list');
   }
   $list = &$drupal_static_fast['list'];
   $sorted_list = &$drupal_static_fast['sorted_list'];
 
-  if (empty($list) || $refresh || $fixed_list) {
+  if (!isset($list) || $refresh || isset($fixed_list)) {
     $list = array();
     $sorted_list = NULL;
-    if ($fixed_list) {
+    // The fixed list may be a completely empty array, thus check for isset().
+    // Calling code may use this to empty out the module list entirely. For
+    // example, unit tests need to ensure that no modules are invoked.
+    if (isset($fixed_list)) {
+    // Re-implant theme registry.
+    // Required for l() and other functions to work correctly and not trigger
+    // database lookups.
+    $theme_get_registry = &drupal_static('theme_get_registry');
+    $theme_get_registry[FALSE] = $this->originalThemeRegistry;

Berdir also mentioned moving verbose() directly to WebTestBase which sounds great.

Berdir’s picture

Also:

(01:55:33 PM) berdir: catch: oh, I guess we could actually move TestBase::verbose() to WebTestBase::verbose() in that follow up
(01:55:50 PM) catch: berdir: that sounds great to me.
(01:56:01 PM) berdir: and possibly merge simpletest_verbose() in there, having that as a separate function makes no sense

To elaborate on simpletest_verbose(), that function is called twice, once from TestBase::run() to inject the current class name and once from TestBase::verbose() to actually generate a verbose log message. If we move that function into the test class, we can get the class name directly and don't need that weird injection stuff. Might be better suited for another issue, though, although I'm not sure if we can actually move it without merging.

Note that there is also a related bug report lying around because verbose() is currently broken on Windows due to \ in class names.

sun’s picture

Priority: Major » Normal
Issue tags: +Testing system
  1. t() is being used by all assertion functions.
  2. Unit tests should be able to use ->verbose(), so I disagree on that. Perhaps UnitTestBase needs a different implementation, but being able to verbose log stuff for debugging is essential for authoring and debugging tests.

This dependency actually is not new... it's just that some recent changes for D8 made it more obvious that the dependency exists (most probably drupal_container() combined with the theme system statics being swapped out with drupal_statics() by #1213536: Non-resettable theme_get_registry() cache causes problems for non-interactive installations).

Berdir’s picture

Well, you can use debug() for "authoring and debugging tests", no?

IMHO, the way TableSortTest uses verbose() is wrong.

catch’s picture

Assert functions - we should change those to use format_string(), same as custom assert messages in tests.

A different implementation for unit tests sounds reasonable.

Given the t() issue I agree this isn't a major bg, would be great if we could have unit tests completely explode if you make a not-really-unit test though.

Berdir’s picture

Berdir’s picture

Status: Active » Needs review
FileSize
6.46 KB

Ok, the attached patch merges simpletest_verbose() into TestBase and makes it UnitTest-safe by removing the l. l() is actually wrong there anyway, as we e.g. don't want language prefixes in there. We already have a full url and don't want to change it.

This duplicates both issues in #6 but one there is already RTBC and major so it might make sense to get that in first and then re-roll this.

Still think that TableSortTest shouldn't be calling verbose() like that but it's not necessary to remove it anymore.

I think we can safely say that unit tests must not use the theme system and t() works as long as locale.module isn't installed.

Status: Needs review » Needs work

The last submitted patch, oopify-verbose-debugging-1611430-7.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
6.58 KB

Ouch. We can't call variable_get('simpletest_verbose', TRUE) during a test, obviously..

sun’s picture

Status: Needs review » Needs work
+++ b/core/modules/simpletest/lib/Drupal/simpletest/TestBase.php
@@ -94,6 +124,16 @@ abstract class TestBase {
   public function __construct($test_id = NULL) {
...
+    if (variable_get('simpletest_verbose', TRUE)) {

I think we rather want to put this into ::run(), since a mere instantiation of a test class does not necessarily mean that tests will be run (and thus, that a verbose directory will be needed).

Berdir’s picture

I wasn't sure about that, I'm fine with moving it (back). The idea of having it in the constructor was that it's something that only needs to be done once. Actually even globally, so it might be possible to move it to a completely different place, like an initial run test preparation, both for the UI/Batch and run-tests.sh. Not sure...

sun’s picture

That central "test run initialization/preparation" is TestBase::run(). ;)

Berdir’s picture

Yes, test run. The verbose directory does not change based on the test class/run, so it could be done once instead of being repeated all the time.

Doesn't matter much, though.

sun’s picture

This patch looks ready to me after:

- moving the verbose directory setup code into ::run().

- Reverting the change/removal in UnitTestBase::setUp(). The theme registry needs to be re-injected, since any call to l() or t() in test assertions would throw a fatal error otherwise.

sun’s picture

Title: Unit tests rely on the database » Verbose debug output in unit tests relies on the database

Better title.

Berdir’s picture

Status: Needs work » Needs review
FileSize
5.72 KB

Re-roll of the requested changes.

IMHO, you can't call a function that relies on the theme registry in a unit test, that is against the concept of unit tests so I'd be ok with throwing a fatal error there (As mentioned above, t() does *not* trigger a theme/database lookup because no languages are enabled).

Status: Needs review » Needs work

The last submitted patch, simpletest-verbose-1611430-16.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
5.71 KB

Ups, re-added the $class = ... line.

sun’s picture

Status: Needs review » Reviewed & tested by the community

Awesome, thanks @Berdir!

I also slept over the removal of theme registry injection in UnitTestBase::setUp() and of course we should remove that if possible. I merely wasn't sure whether verbose debug is the only blocker for removing it. So if you want to quickly add it back, we can remove it in this patch (sooorry). Or we can do that in a follow-up issue.

Berdir’s picture

I'm fine with keeping it out of this patch, we can talk about further improvements in follow-up issues. I'm also going to mark #1588132: Simpletest verbose output url generation should be centralized. as a duplicate of this issue because this now also covers everything that one was about.

sun’s picture

Issue tags: -Testing system
catch’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Testing system

The last submitted patch, simpletest-verbose-1611430-18.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
821 bytes
6.52 KB

Not sure why the patch didn't apply anymore but git rebase was able to merge it just fine :)

Also re-added the removal of the registry injection since I had to do a re-roll anyway.

sun’s picture

Status: Needs review » Reviewed & tested by the community
sun’s picture

sun’s picture

Issue tags: +Needs backport to D7

This patch is required for #1705748: Convert simpletest settings to configuration system, since the current procedural simpletest_verbose() function is completely out of scope of the test that is actually executed, so it is not able to access the runtime configuration of simpletest (of the parent site).

Also, I think this should be backported. (we might have to keep the simpletest_verbose() in the backport though)

catch’s picture

Version: 8.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

Thanks! Committed/pushed to 8.x, moving to 7.x for backport.

Berdir’s picture

Not sure what exactly you want to backport, simpletest_verbose() removal is 90% of this patch. Or do you just want to keep it as a empty wrapper or something like that?

Edit: Ah, do you mean removing the l()?

Berdir’s picture

Status: Patch (to be ported) » Needs review
FileSize
882 bytes

So basically just this, right?

The theme_registry stuff that is removed here hasn't been backported yet.

Berdir’s picture

FileSize
882 bytes

So basically just this, right?

The theme_registry stuff that is removed here hasn't been backported yet.

sun’s picture

Hm. I actually thought of backporting the whole thing - minus the removal of simpletest_verbose().

Essentially, moving/"duplicating" its functionality onto the class, as in D8, but keeping simpletest_verbose() for whatever wonky raw test/script is calling directly into it. (Everything should be using ->verbose(), but you never know...)

sun’s picture

Version: 7.x-dev » 8.x-dev
Status: Needs review » Needs work

Coming from #1608842: Replace list of bootstrap modules, enabled modules, enabled themes, and default theme with config:

+++ b/core/modules/simpletest/lib/Drupal/simpletest/TestBase.php
@@ -457,11 +487,21 @@ abstract class TestBase {
+    $verbose_filename = $this->verboseDirectory . '/' . $this->verboseClassName . '-' . $this->verboseId . '.html';
+    if (file_put_contents($verbose_filename, $message, FILE_APPEND)) {
+      $url = file_create_url($this->originalFileDirectory . '/simpletest/verbose/' . $this->verboseClassName . '-' . $this->verboseId . '.html');

@@ -477,10 +517,16 @@ abstract class TestBase {
   public function run(array $methods = array()) {
...
+      $this->verboseDirectory = variable_get('file_public_path', conf_path() . '/files') . '/simpletest/verbose';

The file_create_url() causes module hooks to be invoked in upgrade tests from ->verbose() calls in the test, before update.php migrated the system data to the current core.

It's not clear to me why we need file_create_url() here -- how can we avoid it?

If necessary, we need to prepare and add a new $verboseDirectoryUrl property that contains the already resolved file URL up to the directory, so we only append the filename.

Berdir’s picture

Status: Needs work » Needs review
FileSize
2.06 KB

This seems to work with manual testing, I think we don't have any verbose output tests.

sun’s picture

Status: Needs review » Reviewed & tested by the community

Thanks! Looks good to me!

webchick’s picture

Version: 8.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

I don't think we can actually add tests for this, sadly, so committed and pushed to 8.x. Thanks!

Back to 7.x for backport.

Stevel’s picture

Issue summary: View changes
Status: Patch (to be ported) » Needs review
FileSize
2.26 KB

An updated patch for Drupal 7, incorporating #31 and #34.

Stevel’s picture

Issue tags: -Needs backport to D7

Removing tag because the backported patch is available.

  • catch committed d29d6b2 on 8.3.x
    Issue #1611430 by Berdir: Fixed verbose debug output in unit tests...
  • webchick committed c10a253 on 8.3.x
    Issue #1611430 follow-up by Berdir: Move call to file_create_url() to...

  • catch committed d29d6b2 on 8.3.x
    Issue #1611430 by Berdir: Fixed verbose debug output in unit tests...
  • webchick committed c10a253 on 8.3.x
    Issue #1611430 follow-up by Berdir: Move call to file_create_url() to...

  • catch committed d29d6b2 on 8.4.x
    Issue #1611430 by Berdir: Fixed verbose debug output in unit tests...
  • webchick committed c10a253 on 8.4.x
    Issue #1611430 follow-up by Berdir: Move call to file_create_url() to...

  • catch committed d29d6b2 on 8.4.x
    Issue #1611430 by Berdir: Fixed verbose debug output in unit tests...
  • webchick committed c10a253 on 8.4.x
    Issue #1611430 follow-up by Berdir: Move call to file_create_url() to...
brad.bulger’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
2.27 KB
1.14 KB

Reroll of 7.x backport

This fixed a PDOException error I was getting from a verbose message in AggregatorUpdatePathTestCase.

David_Rothstein’s picture

Status: Reviewed & tested by the community » Fixed

Committed to 7.x - thanks!

+      $link = '<a href="' . $url . '" target="_blank">' . t('Verbose message') . '</a>';

In theory I think this should use check_plain() on the $url (since it's HTML output) but in practice I suppose it doesn't matter in this case.

  • David_Rothstein committed 039472e on 7.x
    Issue #1611430 by Berdir, brad.bulger, Stevel, sun, catch: Verbose debug...

Status: Fixed » Closed (fixed)

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