Reduce time of Migrate Upgrade tests by not outputting the logs by default

This is due to 📌 Migrate UI upgrade tests should provide the complete log Fixed , I think we should comment out the log settings - they can be uncommented when the test needs debugging, but an extra 10+ minutes waiting for every gitlab run seems a bit much.

Fine with me, I doubt there is much usage of the test in contrib. And I see that there is a flag that can be used to disable the output of the logs, $outputLogs.

Changed this a bit so that the assert on the number of logged errors is kept for the 2 tests Upgrade6 and Upgrade7 where it has value.

Looks much better and the test time is still back down to where it was before the original change.

Trying for a more descriptive title

I'm new to these tests. Took me a moment to understand why we're moving assertLogError() outside the check for if we've written anything to the logs. But then with grep and looking at more context outside of the MR diff, I see that that function relies on $this->expectedLoggedErrors, which is only set in the two functions where assertLogError() is now found.

At the risk of slowing down this important fix, I have to ask: should that method really be something like:

assertNumberOfLoggedErrors(int $expected): void

Why bother with the expectedLoggedErrors property at all? It's only set exactly twice with hard-coded ints each time:

./core/modules/migrate_drupal_ui/tests/src/Functional/d7/Upgrade7Test.php:    $this->expectedLoggedErrors = 27;
./core/modules/migrate_drupal_ui/tests/src/Functional/d6/Upgrade6Test.php:    $this->expectedLoggedErrors = 39;

We never increment or anything. It's basically just a constant. Why not pass it directly where we need it?

That said, I'm going to RTBC this, since my refactoring concern doesn't need to hold this up. If y'all agree the above would be cleaner, but don't want to do it here, it'd be a novice follow-up.

Thanks!
-Derek

I think #8's a good idea for follow-up. When I first saw this I wasn't clear which properties would trigger the assertions vs. not, so having less seems good.

It is looking better now and Test time is also taking less time compared to before

: 📌 Nove MigrateUpgradeExecuteTestBase expectedLoggedErrors property to Active

Committed and pushed 62b65430ed to 11.x and f91aed85e8 to 10.3.x. Thanks!

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