Write UnitTest for MigrationConfigurationTrait and fix Exception Handling

I think so long as #2647470: Write tests goes in, this can be added to core during the beta for 8.1.x or in a patch release of 8.1.x even.

This issue was discussed at the DrupalCon New Orleans migrate critical issue triage meeting. xjm, alexpott, mikeryan, and the rest of the migrate contributors agreed that this is a "nice to have", but shouldn't be considered a critical issue.

-mike

Let's pospone this until #2682585: Rename MigrationCreationTrait as it no longer creates migrations - it configures them gets committed.

@abhishek-anand, still planning to work on this?

per [#268258], MigrationCreationTrait is now MigrationConfigurationTrait.

I was wondering how would one go about testing a class with protected methods? The only example I have to go off of it StringTranslationTrait. This trait is tested via passing a mock created in UnitTestCase::getStringTranslationStub to StringTranslationTrait::setStringTranslation. Is this the correct pattern to follow?

abhishek-anand hasn't responded and it has been 3 weeks, so unassigning.

This is a first stab at setting unit tests, I used the doc here to guide most decisions https://www.drupal.org/docs/8/phpunit/unit-testing-more-complicated-drup.... Any var_dump() commands are still in code because it can be helpful when troubleshooting. I currently run this test by calling phpunit on the new test file:

vendor/bin/phpunit --configuration core core/modules/migrate_drupal/tests/src/Unit/MigrationConfigurationTraitTest.php

This is very much a work in progress and couldn't have gotten as far without the assistance of @phenaproxima

I've been going through some stale migrate issues and this caught my eye. (I've been working on getting better at Unit tests.) This patch is a new approach compared to #18, so there's no interdiff.

I was wondering how would one go about testing a class with protected methods?

There's a common argument that unit testing should only concern itself with public methods. I'm not sure I quite buy the argument in all cases, but that's the approach I ended up taking here. I just test getLegacyDrupalVersion.

One way to test protected methods is to make a ReflectionClass. Another way is to do just what @robpowell did and make a new class that makes public versions of the various protected methods. Once again, I did not take either approach in this patch.

Looking at the trait, we might be ok just with testing the one public function. Most of the other functions are very simple wrappers. The only other non-trivial (protected) functions are getSystemData, createDatabaseStateSettings, and getMigrations. But if we're doing by-the-book unit tests we don't need to deal with those here.

@danflanagan8, thanks for working on this stale issue.

I think the test look good and it tests all paths through getLegacyDrupalVersion except for the two where it throws an exception. Can you add methods to test those?

Thanks for the review, @quietone! Ah, I did shy away from those cases. I've dug back in and updated the patch...and in doing so discovered something interesting. Note that this test fails. (I updated drupal.cli to only run this one test.)

The first try/catch catches a \PDOException whereas the second catches a DatabaseWrapperException. I slogged through the code and it looks to me like Connection::query will throw DatabaseWrapperException, not a \PDOException.

The git history of those two try/catch blocks is very disjointed, and seeing as there was never a unit test, it's not surprising there would be a little boo boo like this.

How to proceed?

Setting back to NR to get feedback on how to proceed.

Yea, I don't know why the wrapper fails but 'exception' => new \PDOException(), works. I do know that when I write such tests I use the specific exception being thrown, That isn't much help though, is it.

I guess there are two approaches that jump out at me:

1. Write the test such that it passes and then address the incorrect exception being caught in a separate issue.
2. Change the scope of this issue and fix it now.

I think since writing the test exposed the issue, fix it here.

I was hoping you would say that, @quietone!

Here's the updated patch. I'll also update the IS and title.

The interdiff does not contain anything about drupal.cli, which is back to normal. Thus the longer filename, since it's not technically the interdiff. :)

@danflanagan8, thanks for fixing that up!

I do like that the clear descriptions for the test cases in the dataProvider array. Often I see and I write tests for exceptions in separate methods but in this case it is much easier to see that all paths are tested.

I think that if we want to add tests for getSystemData, createDatabaseStateSettings, and getMigrations they can be done in a new issue that addresses the protected methods.

Committed and pushed ce4ce0ac356 to 10.0.x and 1f8bcfdba90 to 9.4.x and 3246a76fffb to 9.3.x. Thanks!

This is a bug fix because of the fix to the try catch block so backporting to 9.3.x

This seems to be breaking the HEAD of 10.0.x-dev when testing with PHP 8.1: https://www.drupal.org/pift-ci-job/2278178

TIL

lol

First time my patch has broken HEAD, so that's exciting.

Here's a patch that can be applied on top of the already committed patches that should fix this. No interdiff because this is intended to apply to the dev branches where the previous patch was already applied.

Tests are green on both PHP 8.0 and 8.1 => RTBC

Thanks for the fix. This is a PHP 8.1 change so is failing on all branches.