Closed (fixed)
Project:
Migrate Upgrade
Version:
8.x-3.x-dev
Component:
Code
Priority:
Normal
Category:
Bug report
Assigned:
Unassigned
Reporter:
Created:
20 Feb 2018 at 17:21 UTC
Updated:
9 Mar 2018 at 15:34 UTC
Jump to comment: Most recent, Most recent file
Comments
Comment #2
mikelutzComment #3
mikelutzFix for the test, can't spell, it seems..
Comment #4
mikelutzgrr..
Comment #5
mikelutzI am going to assume the last one will finally successfully fail...
Here's the patch to hopefully fix.
Comment #6
mikelutzComment #7
quietone commented@mikelutz, thanks for the patch, it looks good and works just fine. I do have a few comments.
Can we have a comment here explaining why this is needed?
Not really needed in the test, the runner doesn't change the source values. Let's keep this simple.
Ditto.
But here let's add a previous migration as a dependency for both required and optional and test that they are updated with the prefix as well.
The indentation on the whole class has an extra two spaces.
Don't we have a requirement that there is one class per file? See Object-oriented code.
Ah, this file is using a structure I am not familiar with, so really can't comment.
Can someone else comment about on the structure of the test file, it has two classes and two namespaces, which I am not sure about?
Comment #8
mikelutzI'm happy to add additional comments, and remove the cruft from the test case. There is plenty of precedent for adding one time use protected property/method overrides at the bottom of unit test classes.
@see core/modules/serialization/tests/src/Unit/Normalizer/NormalizerBaseTest.php, core/modules/migrate/tests/src/Unit/destination/PerComponentEntityDisplayTest.php, etc
Using a second namespace is less common, but necessary in this case to override the drush function. There is precedent again
@see core/tests/Drupal/Tests/Core/Session/AccountProxyTest.php, core/tests/Drupal/Tests/Component/Utility/CryptRandomFallbackTest.php
I initially used bracketed namespaces, but I've replaced them with the non-bracketed declarations as they seem to be more standard.
Comment #9
heddnDecent test coverage. We can improve/adjust things over time. Looking good here. Honestly, I'm glad to see test coverage at all, although I would have asked for it if we didn't have any.
Comment #11
heddn