Add kernel tests for prefixing of migrate process errors

Working on this.

Working test pushed to 3400189-add-kernel-tests-migrate-process-errors.

WIP for subprocess message test pushed to sandbox-3400189-subprocess-test because I can't get it to work and have probably done something stupid.

Is MR suppose to be in draft?

Seems to have failed cspell.

It's in draft because I'm stuck on the second part of what I want to test, which is in the sanbox branch -- see my earlier comment. Review of the direction I've taken so far would be helpful.

The Needs Review Queue Bot → tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

Fixed the tests.

The Needs Review Queue Bot → tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

The NIghtwatch test can't be related to this -- this MR is only adding a Kernel test.

Circled back to this one and had to look at other migration kernel tests, mainly for the definition structure, and believe this is covering the feature. Going to mark and also post in #migration slack channel in case needs submaintainer sign off.

I don't think we should switch to using the core SQL ID map. It adds complexity to the test, because we have to remember what might be in the map already, and because we have to write queries. It also adds a performance cost.

This sort of test is a clear case for a in-memory map - we collect the map messages, then get them out. This is the same pattern that tests use when checking log messages.

I agree that I've not set up the map properly - that needs work.

I don't fully agree. If we were using the normal drupal logger, then yes, we can mock that in memory and it's fine. The idmap plugins are their own system, and the Sql Idmap is used 99.9% of the time. I feel like what you want to test here is what message actually makes it into the database, and I think that should include the system that is responsible for storing it in the database. I also don't think the performance costs of the the SQL plugin are significant compared to the costs of running a migration in the test, I feel the additional coverage of the system used to store the messages far outweighs the extra few clock cycles to include it.

That said, I also don't mind keeping tests very specific and isolated. If the goal here is to test the message that gets passed to MigrateIdMapInterface::saveMessage() given a specific process exception thrown in a specific place, I fully support that. We have \Drupal\tests\Migrate\Unit\MigrateSqlIdMapTest::testMessageSave() to test that the message passed to saveMessage() is what makes it to the database.

So if the goal is to test the argument passed to saveMessage is as expected given a process exception, lets do that directly using prophecies, rather than making. a new test module. I created a new MR using that approach. I prefer the prophecies to adding test modules when possible. While I admit this test comes close to the borderline where a complex prophecy setup might make me prefer a test module, it does not cross it for me. The benefit of having the entire test in one class makes this preferable, I believe.

> If the goal here is to test the message that gets passed to MigrateIdMapInterface::saveMessage() given a specific process exception thrown in a specific place,

Yes, that's my intention here. I think we should have test coverage before tackling the two issues I filed about improving the detail that is added to process exception messages.

I'm also in favour of prophecy over test code and keeping code in one place where possible - it didn't occur to me and also I don't know the class structure well enough.

Alright, I think we can close MR5299 and go with MR5462 for this issue then. Leaving as NR from NRQI.

Highlighted which MR is to be used in the issue summary.

The switch to prophecy is very interesting! This should be bookmarked haha

Nothing seemed to break and the coverage seems to be there.

Backported to 10.2.x as it's tests-only.

Committed and pushed 021e0ede07 to 11.x and c2e01926b2 to 10.2.x. Thanks!

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