[meta] Ensure migration tests are meaningful

+∞ for this!!

Migration tests should be ruthless. They should assert everything they can. The fact that the migration system does not validate the migrated data makes this even more crucial.

My feeling is that, if you are testing a migration that you expect to migrate 10 nodes and skip one, then you should assert all of those 10, and every migrated property thereof -- and you should assert that the 11th did not get migrated. The migration tests have greatly improved recently (much more readable and writeable), so there's no particular reason to avoid ruthless testing.

I completely agree that migrated values should be tested using accessor methods rather than raw values. We should always use isPublished() over get('status').

We should also be aware of values which are likely to change frequently in our fixtures (aggregator feed eTags, for example) and make our assertions smarter. In certain cases, it makes more sense to assert a format (like "assert that this is a hex string 32 characters long") than a static value.

Not entirely sure how to proceed, but maybe we can start by writing up a laundry list of complaints/shortcomings in the existing migration tests.

Definitely agree. I know I need to learn this and any guidance would help.

And just today ultimike asked if tests are needed for a very simple change, https://www.drupal.org/node/2598376#comment-10479044.

A laundry list of shortcomings, or if someone prefers, 'the ideal' is a good place to start.

Why improving some test coverage a migrate critical? I agree this is a good issue, someone just needs to go through every test and manually analyse the assertions.

For reference, #2598376: d6_user_settings migration user_register constants don't seem to line up. has an example of testing by building the relevant admin form and checking the values there.

I generally think in lists and phenaproxima mentioned laundry list above so I'm empowered to write up a summary list.

Migration tests

• Will be ruthless.
• Assert every possible value, i.e. all fields on a node, all source values.
• Assert that items skipped are, in fact, skipped.
• Use accessor methods.
• Check actual functionality.
• Handle values that change frequently in any fixture, i.e. aggregator, feed, tags.
• Follow PHPUnit style as appropriate, i.e. assertIdentical($expected, $actual).
• Have documentation with examples.

Are 'use accessor methods' and 'check actual functionality' the same? What's missing?

Thx, chx. Moved the types of tests into a new list. And changed ruthless to rigorous.

Migration tests

• Will be rigorous.
• Assert every possible value, i.e. all fields on a node, all source values.
• Assert that items skipped are, in fact, skipped.
• Handle values that change frequently in any fixture, i.e. aggregator, feed, tags.
• Follow PHPUnit style as appropriate, i.e. assertIdentical($expected, $actual).
• Have documentation with examples.

Preferred order of testing methods

1. Functional testing.
2. Accessor method.
3. Direct property access.

And something like if you aren't using method 1, make a comment in the code?

We removed the migrate-critical tag - this issue as it is is too vague to be actionable. Let's make it a meta and add concrete child tasks to beef up the testing.

There are no concrete child tasks added yet. There has been active work outside of this issue in improving the test coverage of migrate. Does this issue still serve a purpose? Tagging for an IS update.

Moving to NW, not because there is a patch, but because as a plan/meta issue, there is a lot of work needed on this issue.

I think we can close this one.

Back when this issue was opened, the Migrate test suite was a lot shoddier than it is today. After a lot of aggressive refactoring and improvement, Migrate's tests are significantly more hardened than they used to be.