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?
Files: 
CommentFileSizeAuthor
#34 pre-set-verbose-url-1611430-34.patch2.06 KBBerdir
PASSED: [[SimpleTest]]: [MySQL] 42,066 pass(es).
[ View ]
#31 removal-of-l-1611430-30.patch882 bytesBerdir
PASSED: [[SimpleTest]]: [MySQL] 39,353 pass(es).
[ View ]
#30 removal-of-l-1611430-30.patch882 bytesBerdir
PASSED: [[SimpleTest]]: [MySQL] 39,349 pass(es).
[ View ]
#24 simpletest-verbose-1611430-24.patch6.52 KBBerdir
PASSED: [[SimpleTest]]: [MySQL] 39,763 pass(es).
[ View ]
#24 simpletest-verbose-1611430-24-interdiff.txt821 bytesBerdir
#18 simpletest-verbose-1611430-18.patch5.71 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch simpletest-verbose-1611430-18.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#16 simpletest-verbose-1611430-16.patch5.72 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 0 pass(es), 0 fail(s), and 972 exception(s).
[ View ]
#9 oopify-verbose-debugging-1611430-9.patch6.58 KBBerdir
PASSED: [[SimpleTest]]: [MySQL] 36,661 pass(es).
[ View ]
#7 oopify-verbose-debugging-1611430-7.patch6.46 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 36,612 pass(es), 1 fail(s), and 7,771 exception(s).
[ View ]

Comments

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.

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.

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).

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

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

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.

Status:Active» Needs review
StatusFileSize
new6.46 KB
FAILED: [[SimpleTest]]: [MySQL] 36,612 pass(es), 1 fail(s), and 7,771 exception(s).
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new6.58 KB
PASSED: [[SimpleTest]]: [MySQL] 36,661 pass(es).
[ View ]

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

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).

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...

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

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.

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.

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

Better title.

Status:Needs work» Needs review
StatusFileSize
new5.72 KB
FAILED: [[SimpleTest]]: [MySQL] 0 pass(es), 0 fail(s), and 972 exception(s).
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new5.71 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch simpletest-verbose-1611430-18.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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

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.

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.

Issue tags:-Testing system

#18: simpletest-verbose-1611430-18.patch queued for re-testing.

#18: simpletest-verbose-1611430-18.patch queued for re-testing.

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

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

Status:Needs work» Needs review
StatusFileSize
new821 bytes
new6.52 KB
PASSED: [[SimpleTest]]: [MySQL] 39,763 pass(es).
[ View ]

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.

Status:Needs review» Reviewed & tested by the community

#24: simpletest-verbose-1611430-24.patch queued for re-testing.

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)

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.

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()?

Status:Patch (to be ported)» Needs review
StatusFileSize
new882 bytes
PASSED: [[SimpleTest]]: [MySQL] 39,349 pass(es).
[ View ]

So basically just this, right?

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

StatusFileSize
new882 bytes
PASSED: [[SimpleTest]]: [MySQL] 39,353 pass(es).
[ View ]

So basically just this, right?

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

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...)

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.

Status:Needs work» Needs review
StatusFileSize
new2.06 KB
PASSED: [[SimpleTest]]: [MySQL] 42,066 pass(es).
[ View ]

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

Status:Needs review» Reviewed & tested by the community

Thanks! Looks good to me!

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.