Problem/Motivation

If your install has a migrate connection in settings.php, migrate tests end up using that connection instead of mocking a connection. This has to do with the behavior of addConnectionInfo which quietly drops duplicate requests to add connections.

This causes duplicate table failures because its not running against the namespace'd and clean connection.

Proposed resolution

Use the same code simpletest uses to avoid this problem with the default connection and stash the migrate connection into a magically named connection string and the recover it during tear down.

Remaining tasks

Review, commit

User interface changes

n/a

API changes

n/a

Data model changes

n/a

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Bug because tests can fail in certain setups
Issue priority Normal because it isn't a standard setup. (unless you are doing migration work)
Unfrozen changes Unfrozen because it only changes tests and it only affects migration.
Prioritized changes Migrate
Disruption 0. Your standard bug fix.
CommentFileSizeAuthor
#7 migratetests-2555109-7.patch2.4 KBneclimdul
#7 2555109-7.interdiff.txt1.12 KBneclimdul
#4 migratetests-2555109-4.patch2.35 KBneclimdul
#4 2555109-4.interdiff.txt1.32 KBneclimdul
migrate_database_connection_collision.patch2.33 KBneclimdul

Comments

neclimdul created an issue. See original summary.

mikeryan’s picture

mikeryan
he/him
Primary language English
Location Pittsfield, MA, USA
commented

Well, the underlying issue is that MigrateSqlSource in the absence of an explicit database connection defaults to a hard-coded 'migrate' connection, which is a bad thing - connections should really always be explicit. #2552791: MigrateSqlSource should use dependency injection proposes refactoring this whole business, but I'd be happy to at least for the moment remove the 'migrate' default.

neclimdul’s picture

neclimdul
He/Him
commented

I think the code might not have been clear leading to some confusion. The patch just wraps the current code to stash the original migrate connection and then recover it during tearDown. The code is actually stolen from TestBase where it does the same thing for the default connection.

Not really visible in the patch is the logic where we copy the default connection and change the prefix to give the source database its own separate "database" which doesn't need to change to fix this bug.

I'm told there are other issues to handle passing connections better which may allow this whole thing to be replaced with some cleaner code in the future that doesn't muck with the global Database statics but this fixes the problem for now (and helps me bounce between testing automated and manual testing a lot easier).

neclimdul’s picture

neclimdul
He/Him
commented
Issue summary: View changes
StatusFileSize
new 2555109-4.interdiff.txt1.32 KB
new migratetests-2555109-4.patch2.35 KB

changed the order of the methods so its easier to review and added a @todo pointing to the DI issue.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented

I have nits but overall this looks good. It's unfortunate that we need it, but a necessary evil.

  1. +++ b/core/modules/migrate/src/Tests/MigrateTestBase.php
@@ -59,7 +59,29 @@
+   * @todo this can be removed when we don't rely on global connections. https://www.drupal.org/node/2552791

    Nit: URL should be on a new line. 

  2. +++ b/core/modules/migrate/src/Tests/MigrateTestBase.php
@@ -59,7 +59,29 @@
+  private function createMigrationConnection() {

    Should be protected. 

  3. +++ b/core/modules/migrate/src/Tests/MigrateTestBase.php
@@ -59,7 +59,29 @@
+

    Nit: There's an extra line here. 

  4. +++ b/core/modules/migrate/src/Tests/MigrateTestBase.php
@@ -59,7 +59,29 @@
+      throw new \RuntimeException("Bad Database connection state: 'simpletest_original_migrate' connection key already exists. Broken test?");

    "Broken test?" isn't terribly informative. Can this phrase either be made more useful or removed entirely? 

  5. +++ b/core/modules/migrate/src/Tests/MigrateTestBase.php
@@ -78,7 +100,7 @@ protected function setUp() {
+    $this->cleanupMigrateConnection();

    Please add a @see createMigrationConnection() comment. 

  6. +++ b/core/modules/migrate/src/Tests/MigrateTestBase.php
@@ -86,6 +108,17 @@ protected function tearDown() {
+   * Cleans up the test migrate connection.

    This should also mention the DI issue. 

  7. +++ b/core/modules/migrate/src/Tests/MigrateTestBase.php
@@ -86,6 +108,17 @@ protected function tearDown() {
+  private function cleanupMigrateConnection() {

    protected

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Status: Needs review » Needs work
neclimdul’s picture

neclimdul
He/Him
commented
Status: Needs work » Needs review
StatusFileSize
new 2555109-7.interdiff.txt1.12 KB
new migratetests-2555109-7.patch2.4 KB

1) Bah, storm doesn't handle multiline todo's so the link looks unrelated. I made it short enough to fit under 80 but now I don't think it makes as much sense. ok?
2,7) That was intentional. Calling it in child classes would break assumptions about how its called. (overwrite the original connection) TestBase does this as well.
3) It took me 20min to find this... empty lines where required at the top of methods when I started in Drupal... oh the times, the are a changing.
4) Taken from Simpletest's TestBase(along with this entire code block incidentally)
5) Its the only real line in the method. I think its pretty clear the connection and it isn't allowed with the @inheritdoc.
6) done

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Status: Needs review » Reviewed & tested by the community

I keep trying to find new ways to say "RTBC", so I made this: http://i.imgur.com/AE0xTi2.jpg

benjy’s picture

benjy commented 
+++ b/core/modules/migrate/src/Tests/MigrateTestBase.php
@@ -59,7 +59,28 @@
+  private function createMigrationConnection() {

@@ -86,6 +107,19 @@ protected function tearDown() {
+  private function cleanupMigrateConnection() {

I'd rather use protected than private.

neclimdul’s picture

neclimdul
He/Him
commented

So, we actually want this private because being protected means child classes can call it which could lead to accidentally overwriting the previous connection. Simpletests run in separate processes so this shouldn't have a big effect but it could lead to some weird problems.

webchick’s picture

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

Makes sense to me.

Committed and pushed to 8.0.x. Thanks!

  • webchick committed 39902b8 on 8.0.x 
    Issue #2555109 by neclimdul, phenaproxima: MigrateTests incorrectly...

Status: Fixed » Closed (fixed)

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