Problem

  • DrupalWebTestCase::setUp() performs a range of different operations, which cannot be invoked individually.

Goal

  • Allow tests to prepare individual parts of the testing environment.

Related issues

Attached patch is extracted from #1364798: Impossible to generate meaningful diffs of upgrade db dumps, which I had to duplicate to a large extent for some other patches already, so pretty much proven to be the right way forward.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

RobLoach’s picture

Status: Needs review » Needs work
+++ b/core/modules/simpletest/drupal_web_test_case.phpundefined
@@ -1310,6 +1303,18 @@ class DrupalWebTestCase extends DrupalTestCase {
+    $callbacks = array();
+  }
+
+  protected function prepareEnvironment() {
+    global $user, $language_interface, $conf;

Once we have a docblock, I'd consider this RTBC!

-19 days to next Drupal core point release.

xjm’s picture

Status: Needs work » Needs review
+++ b/core/modules/simpletest/drupal_web_test_case.phpundefined
@@ -1317,38 +1322,20 @@ class DrupalWebTestCase extends DrupalTestCase {
-    // Set to English to prevent exceptions from utf8_truncate() from t()
-    // during install if the current language is not 'en'.
-    // The following array/object conversion is copied from language_default().
-    $language = (object) array(
-      'langcode' => 'en',
-      'name' => 'English',
-      'direction' => 0,
-      'enabled' => 1,
-      'weight' => 0,
-    );

I wasn't sure why this hunk was being removed and not added back anywhere else, but per sun, this is orphaned code and has been replaced by the $language_interface global (see below).

+++ b/core/modules/simpletest/drupal_web_test_case.phpundefined
@@ -1428,17 +1441,17 @@ class DrupalWebTestCase extends DrupalTestCase {
     // Set up English language.
-    unset($GLOBALS['conf']['language_default']);
+    unset($conf['language_default']);
     $language_interface = language_default();

See also #1510686: Replace remaining references to global $language_interface with drupal_container().

sun’s picture

Added phpDoc for prepareEnvironment() and took the opportunity to plug way more comments into these methods.

Status: Needs review » Needs work

The last submitted patch, drupal8.simpletest-setup.3.patch, failed testing.

RobLoach’s picture

Status: Needs work » Reviewed & tested by the community

Like xjm mentioned, the language stuff will likely change once the container hits it.

RobLoach’s picture

Status: Reviewed & tested by the community » Needs work

Whoops, thought that one would've passed

sun’s picture

Status: Needs work » Needs review
FileSize
10.46 KB

Sorry, I mistakenly moved drupal_save_session() into prepareEnvironment(), but that could not work, since dss() uses a drupal_static() to retain the passed value, but all statics are reset during setUp(). Reverted that.

xjm’s picture

Status: Needs review » Reviewed & tested by the community

I really like this patch.

catch’s picture

Status: Reviewed & tested by the community » Fixed

Looks good to me too, committed/pushed to 8.x.

catch’s picture

Status: Fixed » Needs work

Hmm, no 8.x branch tests just broke and it looks like drupal_static_reset() in setUp() so I've reverted this to see if it fixes it.

sun’s picture

Status: Needs work » Needs review

#7: drupal8.simpletest-setup.5.patch queued for re-testing.

sun’s picture

ah, yes - I'm pretty sure that #1213536-32: Non-resettable theme_get_registry() cache causes problems for non-interactive installations (#32, more conclusions in #63) is the reason for that.

So let's try this.

xjm’s picture

Status: Needs review » Reviewed & tested by the community

That looks good as well.

catch’s picture

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

Yeah moving the drupal_static_reset() earlier is the right fix.

However the patch no long applies now, 2 hunks failed - tagging Novice for the re-roll.

sun’s picture

Status: Needs work » Reviewed & tested by the community
Issue tags: -Novice
FileSize
10.44 KB

Not exactly novice to re-roll.

catch’s picture

Status: Reviewed & tested by the community » Fixed

Thanks! Committed/pushed to 8.x.

sun’s picture

Thanks a lot everyone for helping me to get this in.

Separating these weird operations into proper individual methods possibly was the best thing that happened to DrupalWebTestCase recently.

In #1404198: Separate database cache clearing from static cache clearing and data structure rebuilding I just realized that it's totally inane to changeDatabasePrefix() before prepareEnvironment(), 'cos the latter attempts to backup tons of stuff by querying the parent site. The fact that this worked was solely caused by a weird mix of environment information and lots of cached variables. (will be fixed over there)

Without this patch, this would have possibly taken ages to figure out. :)

sun’s picture

Status: Fixed » Needs review
FileSize
7.89 KB

Forgot to adjust the upgrade path tests accordingly.

This blocks #1496534: Convert account settings to configuration system

And: Yay for removing duplicated code! :)

sun’s picture

@catch just informed me that #18 additionally fixes #1353516: Tests for: Tables are not deleted when a module is disabled during test execution

That said, let me also include the follow-up fix from #1404198: Separate database cache clearing from static cache clearing and data structure rebuilding

So in turn the patches in here should be complete and... tentatively marking for backport :)

sun’s picture

Status: Needs review » Reviewed & tested by the community

Sorry for rtbc'ing my own patch, but I'd really prefer the change for setUp() to land with this patch instead of #1404198: Separate database cache clearing from static cache clearing and data structure rebuilding

sun’s picture

Backport info: @Dries committed this change as part of #1404198: Separate database cache clearing from static cache clearing and data structure rebuilding:

+++ b/core/modules/simpletest/drupal_web_test_case.php
@@ -1415,6 +1415,9 @@ class DrupalWebTestCase extends DrupalTestCase {
     // Create the database prefix for this test.
     $this->prepareDatabasePrefix();
 
+    // Prepare the environment for running tests.
+    $this->prepareEnvironment();
+
     // Reset all statics and variables to perform tests in a clean environment.

@@ -1425,9 +1428,6 @@ class DrupalWebTestCase extends DrupalTestCase {
     $this->changeDatabasePrefix();
 
-    // Prepare the environment for running tests.
-    $this->prepareEnvironment();
-
     // Preset the 'install_profile' system variable, so the first call into

Re-rolled against HEAD.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, drupal8.simpletest-setup.21.patch, failed testing.

sun’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
7.88 KB

Plenty of conflicts due to #1352000: Forward-port upgrade test clean-ups from 7.x to 8.x

Re-rolled against HEAD.

marcingy’s picture

+1 from me for RTBC.

alexpott’s picture

+1 from me for RTBC - unblocks issues for #1496534: Convert account settings to configuration system

alexpott’s picture

Through testing this patch with #1496534: Convert account settings to configuration system I've discovered an interesting issue with configFileDirectory which is set using originalFileDirectory which in turn depends on the file_public_path variable. The end result is that if a test does two upgrades in a test we have a problem as it tries to write to a directory like "sites/default/files/simpletest/228952/simpletest/7462732". This happens in Language upgrade test.

To fix this we just need to reset the file_public_path directory in the test teardown:

    // Reset public files directory.
    $GLOBALS['conf']['file_public_path'] = $this->originalFileDirectory;
marcingy’s picture

Priority: Normal » Major

The change in #26 makes sense, this issue is pretty much blocking all work on CMI upgrades so moving to major and getting ready to be shot ;)

catch’s picture

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

Hmm 'cos I'd committed this I kept thinking it was the D7 backport, committed/pushed the followup to D8, this definitely isn't blocking CMI in D7, but agree it'd be nice to backport if possible to keep the test framework in sync.

tim.plunkett’s picture

Status: Patch (to be ported) » Needs review
FileSize
8.35 KB

Here's an attempt, I manually resolved the conflicts as best as I could, but I didn't test it at all.

Status: Needs review » Needs work

The last submitted patch, drupal-1541958-28.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
18.87 KB

This backport is absolutely required now, since we have test regressions in D7:
#1213536: Non-resettable theme_get_registry() cache causes problems for non-interactive installations
#1587990: Regression: Simpletest always runs with clean URLs off

I've manually performed the identical, cumulative changes to DrupalWebTestCase and UpgradePathTestCase.

sun’s picture

Category: task » bug
Priority: Normal » Critical

Bumping priority, since some contributed module tests have been reported to be failing currently.

catch’s picture

Status: Needs review » Reviewed & tested by the community

Marking this RTBC, it looks like a big change, but it's really just moving code around and removing some duplication, however having this reorganisation makes it a lot easier to fix bugs in the testing system itself (which we're finding lots of at the moment).

sun’s picture

#31: drupal.simpletest-setup.31.patch queued for re-testing.

David_Rothstein’s picture

I read through this very carefully and it looks like a great cleanup. This is a big enough reorganization that I'm a little afraid it will manage to break something, but I can't see what, and we know it fixes some things that are already broken :) And I agree that having this sanely organized (and consistent with D8) is worth doing in D7.

I'm planning to commit this to 7.x tomorrow. In the meantime, if someone wants to fix the code comments that refer to "Drupal\Core\Utility\CacheArray" (that's a remnant from the D8 patch) that would be lovely; if not, I'll fix it on commit.

Also noticed a couple smaller documentation issues that would be good to fix in a followup (I think they apply to D8 too):

  1. +   * being performed for the test. It is also used as user agent HTTP header
    

    "as the user agent HTTP header"

  2. +   * Backups various current environment variables and resets them, so they do
    

    "Backs up"

  3. +    // Login as uid 1.
    

    "Log in"

  4. +    $this->public_files_directory = $this->originalFileDirectory . '/simpletest/' . substr($this->databasePrefix, 10);
    +    $this->private_files_directory = $this->public_files_directory . '/private';
    +    $this->temp_files_directory = $this->private_files_directory . '/temp';
    

    Shouldn't these properties be camel case? And shouldn't they (and other properties introduced by the patch) be declared and documented at the top of the class (although I suppose we have plenty of existing ones that aren't either)?

sun’s picture

Attached patch changes DrupalCacheArray.

Everything else I want to stay identical between D8 and D7 to ease future patches and backports. Thus, those minor fixes are most likely not going to happen, neither now nor in the future. The current Simpletest module basically transitioned into "maintenance fixes only" state. See current critical bug core queue to see only a few examples. We'll either rewrite it completely or replace it entirely, but we'll definitely not waste time on correcting typos and grammar. ;) A more in-depth write-up and issue on that topic will follow soon.

David_Rothstein’s picture

Status: Reviewed & tested by the community » Needs work

Oops. I was going to commit this, but then right before I did, something jumped out at me:

-    // Remove all prefixed tables (all the tables in the schema).
-    $schema = drupal_get_schema(NULL, TRUE);
-    foreach ($schema as $name => $table) {
-      db_drop_table($name);
+    // Remove all prefixed tables.
+    $tables = db_find_tables($this->databasePrefix . '%');
+    foreach ($tables as $table) {
+      if (db_drop_table(substr($table, strlen($this->databasePrefix)))) {
+        unset($tables[$table]);
+      }
+    }
+    if (!empty($tables)) {
+      $this->fail('Failed to drop all prefixed tables.');
     }

This change will break things in the case where the parent site (running the tests) uses a database prefix itself. Because $this->databasePrefix (despite its name) does not represent the actual database prefix in use; rather it represents the database prefix relative to the parent site's. This can be seen from the code in changeDatabasePrefix():

    // Clone the current connection and replace the current prefix.
....
      $connection_info[$target]['prefix'] = array(
        'default' => $value['prefix']['default'] . $this->databasePrefix,
      );

So, the effect is that after this patch, running tests on a site with a database prefix will never clean up the tables and never warn you about it either. Each time you run the tests, you'll just add more and more tables to your database.

When fixing this maybe it would also be a good idea to ensure that the $this->fail() call above catches this scenario (i.e., if $tables is empty before the foreach() loop, it should fail then also).

David_Rothstein’s picture

Version: 7.x-dev » 8.x-dev

Actually, this regression presumably exists in D8 now also, so it needs to get fixed there before backporting the whole thing.

David_Rothstein’s picture

Issue tags: +7.15 release blocker

Tentatively marking this as a D7 release blocker, since it fixes some regressions in the testing system.

catch’s picture

Issue tags: -7.15 release blocker

Hmm that's part of #1353516: Tests for: Tables are not deleted when a module is disabled during test execution which was marked duplicate of this issue.

Since we're already leaving left-over tables on every site that ever runs the full test suite, not just prefixed ones, I'd be tempted to go ahead and commit this, then re-open that issue against 8.x again for backport to try to fix the table deletion once and for all (this originally came up in #1212992: Prevent tests from deleting main installation's tables when parent::setUp() is not called. Leaving status how it is for the moment though.

David_Rothstein’s picture

Issue tags: +7.15 release blocker

Huh, that's interesting. Well, I'd be OK with doing it either way, I guess. Definitely want to make sure that this whole thing gets sorted out before Drupal 7.15 is released, though...

(Also, fixing tag removed during crosspost.)

sun’s picture

Status: Needs work » Needs review
Issue tags: -7.15 release blocker
FileSize
976 bytes

Hm. Good point.

As visible in the patch, the code is actually incorporated/merged from the upgrade test case, which inherently means that anyone who would have run a upgrade test case on a prefixed (parent site) database left plenty of stale tables behind. But alas, no one but the d.o testbot runs upgrade tests (and that's not using a prefix either).

Let's see whether attached patch works. (will most likely kill the testbot if it doesn't)

Also, testing system patches are pretty much release-agnostic - they take effect immediately (due to the d.o testing infrastructure) and it's safe to assume that most developers running tests are also running on a git checkout (or at least dev snapshot). Thus, removing the release blocker tag. (This doesn't change the importance of this patch though.)

catch’s picture

I've re-opened #1353516: Tests for: Tables are not deleted when a module is disabled during test execution for explicit testing of the disabled modules issue.

Status: Needs review » Needs work

The last submitted patch, drupal8.simpletest-setup.44.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
978 bytes

Stupid copy+paste error, my bad.

Status: Needs review » Needs work

The last submitted patch, drupal8.simpletest-setup.47.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
1.78 KB

Hopefully this cuts it.

Status: Needs review » Needs work

The last submitted patch, drupal8.simpletest-setup.49.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
1.11 KB

Something weird is going on when running tests within the testbot. The tables seem to be prefixed twice (e.g. simpletest437824simpletest488278actions) which means that the substr logic below still ends up with a table that's prefixed once, fails to delete it and then we get 3 more fails.

No idea what the patch #49 does, that makes no sense to me. Instead, we either need to:

- Don't do that double-prefixing, but I think that makes sense, as we *are* running a test within a test.
- Change the table delete logic to be able to cope with that double prefixing, by using the actual prefix, as the code above now does (which also means that we never found those previously and that's why this error didn't show up before).

The attached patch implements the second suggestion and reverts the change from #49.

sun’s picture

Status: Needs review » Reviewed & tested by the community

Thanks!

catch’s picture

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

OK that looks good. Committed/pushed.

Back to 7.x port I think.

effulgentsia’s picture

Status: Patch (to be ported) » Needs review
FileSize
19.37 KB

This is just a mechanical combo of #36 plus #51 backported to D7. I have not reviewed it though, as I'm coming into this issue late, and just trying to help nudge the criticals along to get below threshholds.

effulgentsia’s picture

+++ b/.gitignore
@@ -4,3 +4,4 @@ sites/*/settings*.php
 # Ignore paths that contain user-generated content.
 sites/*/files
 sites/*/private
+/patches/

Sorry. Stray hunk from local .gitignore changes. This removes that.

David_Rothstein’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +7.15 release blocker

Looks good to me, so I'm moving this back to RTBC. If someone else wants to give it a final review before I commit it, that would be great though.

I really do want to make sure we get this into the next Drupal 7 release... Although it's true that developers writing tests for core (and some contrib modules) won't be affected by that since they'll run the latest core snapshot, I don't think that's true for a lot of contrib developers and especially for people writing site-specfic tests for custom modules. In those situations people tend to use point releases of Drupal core only.

David_Rothstein’s picture

Issue tags: +Avoid commit conflicts

Adding another tag also, because this patch moves lots of code around so it would be a pain to have to reroll it.

Leeteq’s picture

I understand and appreciate the urgency of this issue from the point of view of developers.
At the same time, various comments here makes this jump out as a typical issue where it may be more aspects that are easily forgotten "in a rush", which may cause "unforeseen" new issues that might screw up 7.15.

Is it really worth that risk?

I fail to see that this is "critical" and not "major", and from the point of view of the value of having a really stable 7.15 release, I am not sure this issue "deserves" the position as a "7.15 release blocker".

sun’s picture

Status: Reviewed & tested by the community » Needs review

Yes, definitely needs to get in for D7, because some other changes caused regressions in contrib tests and this patch fixes them; see #31.

However, I need to carefully review the backport to make sure the code is identical.

David_Rothstein’s picture

@Leeteq, if you can find something specific that's wrong with this patch, now is definitely the time to speak up :) But this issue is fixing some serious bugs, so it makes sense to get it in as soon as we reasonably can.

I did think of one additional problem that might be worth considering (Drupal 7 only). This patch adds three new methods to DrupalWebTestCase:

protected function prepareDatabasePrefix()
protected function changeDatabasePrefix()
protected function prepareEnvironment()

That would not be too bad normally, but DrupalWebTestCase is extended by literally hundreds (probably thousands) of other classes in Drupal 7. So what are the chances of a method name conflict, i.e. an accidental override? (Probably unlikely for the first two, but "prepareEnvironment" is a pretty generic-sounding name.)

A conflict would be particularly bad here because you'd accidentally override one of the critical parts of the simpletest setup procedure, meaning that you might be able to start leaking things into the parent site when the test is run.

So one simple way to protect against this would just be to do this:

final protected function prepareDatabasePrefix()
final protected function changeDatabasePrefix()
final protected function prepareEnvironment()

This means that in the unlikely event of a conflict, you'd get an easy-to-fix fatal error rather than allowing the tests to run in a potentially broken/dangerous environment. (And I think it's probably fine to prevent these new helper methods from being intentionally overridden in Drupal 7; doesn't seem like there'd be too big of a use case for that, and it's a new feature anyway.)

sun’s picture

Hm. Note that if #1364798: Impossible to generate meaningful diffs of upgrade db dumps is going to happen, then that is going to extend prepareEnvironment(), so at least in D8, they couldn't be final, and I'd ideally like to keep these fundamental things identical between D7 and D8.

David_Rothstein’s picture

Perhaps another way would be to have each of the submethods set a variable at the end, like $this->preparedDatabasePrefix = TRUE, and then have the core setUp() methods check those, and if it doesn't find them, stop what it's doing and return.

That way, none of the dangerous code will run, and simpletest will record a failure (because $this->setup will have never been set to TRUE).

That actually might not be a bad idea for Drupal 8 either. Because we have had that general problem in the past where something goes wrong with setUp() and bad things leak into the parent site (which I believe is why the $this->setup variable was introduced in the first place), and this is really just an extension of that.

webchick’s picture

#55: drupal.simpletest-setup.54.patch queued for re-testing.

sun’s picture

Status: Needs review » Needs work

@David_Rothstein's point on potentially overridden methods is still valid.

Not really for D8, because for D8 I'd just simply say that if your code unintentionally overrides any of these methods, then your code is broken, period. If you blow up your site or some testbot with that code, then that's your fault.

However, for D7, the situation is completely different, since the methods did not exist before. Since I've personally written tons of test case classes that are adding plenty of custom methods, I can relate very well to the possibility that these method names might already exist.

At the same time, setUp() itself might have been overridden (entirely) by a test case class. Thus, adding some properties that intend to ensure that the methods have been executed is not really a definite answer/solution, since the check would have to be contained in setUp(). Replacing the setUp() method is definitely not uncommon and there's nothing wrong with that. The upgrade path tests in Drupal core serve as a perfect example for that.

Of course, we could move that check one level higher into TestBase::run(), before it goes ahead and executes the test* methods. However, technically, that just moves the problem space one level higher :P

So I guess the backport boils down to making a compromise somewhere, but I'm not yet sure where we want to draw the line.

effulgentsia’s picture

This seems like a good case for in D7 to make the new methods private instead of protected. I know we try to avoid private methods, but this is exactly why languages distinguish between private and protected: so that you can change a base class without worrying about introducing an API change to your extending classes.

David_Rothstein’s picture

I don't think we can make them private because the upgrade tests (which extend the base class) are calling these methods too...

At the same time, setUp() itself might have been overridden (entirely) by a test case class. Thus, adding some properties that intend to ensure that the methods have been executed is not really a definite answer/solution, since the check would have to be contained in setUp(). Replacing the setUp() method is definitely not uncommon and there's nothing wrong with that. The upgrade path tests in Drupal core serve as a perfect example for that.

Well, any setUp() method which extended the base class and decided to start calling these new methods would be expected to check the properties also (just like they're expected to fiddle with $this->setup now)... That won't break anyone's existing code, since it's only a requirement if you decide to rewrite your existing setUp() method to call the new helper methods, right?

chx’s picture

The way to go is to add a __drupal prefix to these methods because everyone knows those are PHP magic and reserved however we can rest assured that no PHP version any time soon will introduce __drupalPrepareDatabasePrefix and so on.

mgifford’s picture

I'm really hoping we'll see a 7.15 release this week. So, I'm looking at the 7.15 release blocker list and seeing what is outstanding.

I know I can't provide meaningful answers to @sun and others about how to insert a compromise around setUp(), but who can.

sun’s picture

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

Let's do this:

David_Rothstein’s picture

That works for me, and I think @chx's suggestion would work fine too. Perhaps this one is slightly preferred since it allows us to keep D7 and D8 in sync?

Either way, it would be great to get one of them in ASAP, and this one has a patch so I'll review it :)

  1. The patch doesn't change UpgradePathTestBase (which calls these methods too)... shouldn't it?
  2. Also, it doesn't have any protection for prepareDatabasePrefix()... Maybe it doesn't need a separate variable for that, but it would at least be a good idea to have changeDatabasePrefix() verify that the database prefix exists before trying to use it.
chx’s picture

Just returning FALSE would mean that the test is silently skipped isn't it?

David_Rothstein’s picture

I believe it should work, because as long as the setUp() method returns early (without setting the $this->setup variable to TRUE), the run() method will notice the absence of $this->setup and record a failure.

Haven't actually tried it out, though.

David_Rothstein’s picture

Actually, it looks like the full code is this:

if ($this->setup) {
  try {
    $this->$method();
    // Finish up.
  }
  catch (Exception $e) {
    $this->exceptionHandler($e);
  }
  $this->tearDown();
}
else {
  $this->fail(t("The test cannot be executed because it has not been set up properly."));
}

So maybe that means the changes the above patch made to tearDown() aren't actually necessary? If setUp() fails to complete correctly, it looks like tearDown() doesn't ever get called.

David_Rothstein’s picture

To keep this issue moving, here's a reroll of #69 with the changes from #70 and #73 incorporated.

Really want to get this one in and the whole thing backported to Drupal 7 as soon as possible :)

Status: Needs review » Needs work

The last submitted patch, simpletest-1541958-74.patch, failed testing.

David_Rothstein’s picture

Those failures look suspicious. Let's try again.

David_Rothstein’s picture

Status: Needs work » Needs review

#74: simpletest-1541958-74.patch queued for re-testing.

sun’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
824 bytes
5.38 KB

Fine with me. I merely restored one additional check in WebTestBase::tearDown() for a very good reason.

catch’s picture

Version: 8.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Needs work

OK committed/pushed to 8.x, moving back to 7.x again.

David_Rothstein’s picture

Status: Needs work » Needs review
FileSize
2.87 KB
20.12 KB

OK, here's a combination of #55 and #78 for Drupal 7.

However, I left out the teardown() change from #78... I think that's OK for Drupal 8, but for Drupal 7, we're explicitly not assuming that everyone is going to rewrite their custom setUp() methods to call the new helper functions, right? The old way (having the code still live inline in the setup method) still needs to work. Therefore, putting that check in the tearDown method could cause problems, if someone is totally overridding setUp() but not overriding tearDown().

I think it's OK, though. If $this->setup was tricked into TRUE, you already have lots of problems on your site as is... So although it's a good check to perform, it shouldn't be necessary to add for Drupal 7, right?

David_Rothstein’s picture

Bump... anyone able to do a final review and RTBC of this?

It was essentially RTBC before, so hopefully this will be quick. My goal is to get this patch committed ASAP (by the weekend at the latest) so we can release Drupal 7.15 on August 1.

David_Rothstein’s picture

Bumping again...

To be honest, if this is the last remaining 7.15 release blocker (which it looks like it might be), I'm not sure I'd hold off the release for this patch alone. It was borderline anyway, and it only affects Simpletest, rather than actual functionality on live sites.

But it would definitely be a bummer to still have all these regressions in the next release of Drupal 7, so I'm still really really really hoping we can get it in, let's say by Sunday at the latest.

Melissamcewen’s picture

I have applied this patch (#80) to my own Drupal 7 installation and have not encountered errors. Is there anything specific I should be looking for?

Berdir’s picture

Status: Needs review » Reviewed & tested by the community

Ok.

I've looked over the changes again, applied the patch locally and run a bunch of my own tests (I do have some creative ones, major (6.x -> 7.x) and minor module upgrade tests, ... ) and did not find any issues.

And even if we actually manage to break a test or two out there, well, worse things have happened and 7.14 introduced way bigger changes IMHO.

(Takes deep breath)

I think this is RTBC.

David_Rothstein’s picture

Awesome, thanks!

So, if no problems come up in the meantime, I'll plan to commit this on Sunday with the last batch of Drupal 7.15 patches...

@Melissamcewen, thanks for testing also! Specific things to look for would be to make sure that various simpletests are actually able to run successfully (from both the UI and the command line), and especially that nothing from the simpletest run is able to "leak out" into the parent site. For example, after the test run finishes, all the simpletest temporary tables should be cleaned out from the database, and the main site itself should not have changed in any way, etc. I think we're pretty good there based on previous testing, but that's the most dangerous thing that I can imagine happening from a patch like this :)

sun’s picture

I'd recommend to either commit this super-early before the new release (i.e., now), or postpone the commit to after the new release.

The patch visually looks complete, but I did not perform a full visual diff between the classes in D7 and D8.

David_Rothstein’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: +7.15 release notes

I was at CapitalCamp so I didn't get a chance to commit this until today unfortunately, but we still have several days for people to catch any problems before the new release.

I did some final testing and reviewing of this, as well as visually comparing the D7 and D8 methods to make sure they're the same (or at least as much as I could given that some other changes went into D8 after this), and everything checked out, so...

Committed to 7.x! http://drupalcode.org/project/drupal.git/commit/568611e

Thanks to everyone who put a lot of work into this.

I did notice one small issue:

+    // Remove all prefixed tables.
+    $tables = db_find_tables($this->databasePrefix . '%');
+    $connection_info = Database::getConnectionInfo('default');
+    $tables = db_find_tables($connection_info['default']['prefix']['default'] . '%');

The double-definition of $tables is unnecessary there (and not present in D8). But it's also harmless so no need to hold up the patch... Maybe we can do a quick followup issue for that, though?

David_Rothstein’s picture

Title: Split setUp() into specific sub-methods » Change notification for: Split setUp() into specific sub-methods
Version: 7.x-dev » 8.x-dev
Category: bug » task
Issue tags: -Avoid commit conflicts

Actually, I think this needs a change notification? I believe the end result of the D8 patch is that people are required to rewrite their custom setUp() methods to use the helper functions, or otherwise the standard core tearDown() method will not work with their code (in D7 it might be recommended to rewrite these, but not required)...

If so, that sounds like change notification material.

David_Rothstein’s picture

Status: Fixed » Active

Er...

sun’s picture

That duplicate line will be cleaned up in #1563620: All unit tests blow up with a fatal error

Berdir’s picture

Something like this: http://drupal.org/node/1740112?

Berdir’s picture

Status: Active » Needs review
tim.plunkett’s picture

Status: Needs review » Needs work

At the least these should link to the actual methods themselves.

Ideally it would have a summary of how each should be used, and what parts of an example setUp() would be moved into them, with code examples.

Berdir’s picture

Status: Needs work » Needs review

Ok, updated it a bit. I think giving detailed instructions on how to use them is overkill, I assume that test cases that overwrite setUp() completely make up only a tiny fraction ( I don't know or have any) and if you do something like this then this is the risk you take ;)

tim.plunkett’s picture

Title: Change notification for: Split setUp() into specific sub-methods » Split setUp() into specific sub-methods
Version: 8.x-dev » 7.x-dev
Category: task » bug
Status: Needs review » Fixed

Looks good to me.

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