Migration rule failure for titleText.textProperty

[pixelzoom]

I can't make sense of this Migration wrapper error. @zepumph can you translate for me? What needs to be done?

To reproduce:

1. Start the sim in the Migration wrapper from phetmarks, and add query parameter &strictMigrationRules
2. Press the "Migrate Now" button. These console errors occur:

Assertion failed:  MigrationRule did nothing in a setState call: 
    ConvertToStringsStateForLocale: Renames phetioIDs containing naturalSelection.homeScreen.view.titleText.textProperty -> naturalSelection.general.model.strings.naturalSelection.naturalSelection.titleStringProperty. The phetioID was renamed between versions, and the PhET-iO Element's state was forwarded to the new name for dynamic locale and string support in simulations. Most likely nothing else is needed for this migration rule, but if customizing in the previous version, it would be good to double check that the text is still customized in the new version.
window.assertions.assertFunction @ assert.js:24
ensureAllMigrationRulesDidSomething @ MigrationEngine.ts:197
(anonymous) @ MigrationEngine.ts:91
process @ MigrationEngine.ts:91
loadStandardPhetioWrapperFromHTML @ StandardPhetioWrapper.ts:315
await in loadStandardPhetioWrapperFromHTML (async)
loadStandardPhetioWrapperFromHTML @ Client.ts:1524
(anonymous) @ ?sim=natural-selection&locales=*&keyboardLocaleSwitcher&phetioDebug=true&phetioWrapperDebug=true&strictMigrationRules:241

assert.js:28 Uncaught (in promise) Error: Assertion failed: MigrationRule did nothing in a setState call: 
    ConvertToStringsStateForLocale: Renames phetioIDs containing naturalSelection.homeScreen.view.titleText.textProperty -> naturalSelection.general.model.strings.naturalSelection.naturalSelection.titleStringProperty. The phetioID was renamed between versions, and the PhET-iO Element's state was forwarded to the new name for dynamic locale and string support in simulations. Most likely nothing else is needed for this migration rule, but if customizing in the previous version, it would be good to double check that the text is still customized in the new version.
    at window.assertions.assertFunction (assert.js:28:13)
    at MigrationEngine.ensureAllMigrationRulesDidSomething (MigrationEngine.ts:197:19)
    at MigrationEngine.ts:91:56
    at Array.forEach (<anonymous>)
    at MigrationEngine.process (MigrationEngine.ts:91:28)
    at StandardPhetioWrapper.loadStandardPhetioWrapperFromHTML (StandardPhetioWrapper.ts:315:38)
    at async Client.loadStandardPhetioWrapperFromHTML (Client.ts:1524:20)
    at async ?sim=natural-selection&locales=*&keyboardLocaleSwitcher&phetioDebug=true&phetioWrapperDebug=true&strictMigrationRules:241:13
window.assertions.assertFunction @ assert.js:28
ensureAllMigrationRulesDidSomething @ MigrationEngine.ts:197
(anonymous) @ MigrationEngine.ts:91
process @ MigrationEngine.ts:91
loadStandardPhetioWrapperFromHTML @ StandardPhetioWrapper.ts:315

[pixelzoom]

I'm also confused about the expected behavior of ?strictMigrationRules in general. I realize that there are some cases when a failure occurs that are not really things that need to be addressed (like perhaps this issue). But I don't know how to identify which failures need to be addressed vs ignored, and how to communicate/document which failures can be ignored. And continuing to fail assertion that can be ignored seems like an on-going maintenance/QA problem -- do we need some way to mark these as "do not fail assertion because it may legitimately do nothing"? Or are we expected to "manually" remember which failures don't need to be addressed?

[zepumph]

Ahhh, very good bug here. Over in https://github.com/phetsims/phet-io-wrappers/issues/521 we decided that when having a many-to-one mapping in migration rules, that you should not overwrite a custom value of the mapping with a default value. This means that this rule is potentially not going to do anything, since it is the second rule to map to the sim's titleStringProperty:

https://github.com/phetsims/phet-io-sim-specific/blob/db30646cde1ed70c3822692e0f4eed47646282ad/repos/natural-selection/js/natural-selection-migration-rules.ts#L93-L95

So we need a way to opt out of this assertion for certain rules, to help assist in testing. I'll work something out.

[zepumph]

Ok, so I added canDoNothing to bypass the ensureAllMigrationRulesDidSomething strict checker. 20 rules in NS needed this. I'm unsure that this is the best path forward, but I can see that being explicit about these would help everyone understand the rule that much better. Since many-to-one mappings (mostly for strings) are incredibly confusing, and it is good to understand how overwriting may occur. @pixelzoom I also covered https://github.com/phetsims/natural-selection/issues/341 in the above commit, let's discuss what you think. Migration is working without error for me with the above commit.

[pixelzoom]

Looks good, confirm with in Migration wrapper with &strictMigrationRules. Closing, thanks!