Would it be possible to add some inline comments to the different parts of this large for loop or split it up so that is clear what the intent is? It is quite difficult to review given that there is no motivation given for what we do here.

It looks like 'ai' isn't really used so why not e.g foreach instead?

Since this is inside a loop I would recommend moving tokens outside the loop and just do clear here to avoid GC pressure.

Also this list may be moved to outer scope to avoid allocating on every iteration.

same with this list

This dict could also be outer scope?

I do not understand what the goal is here, could you add inline comments explaining?

Also code from here to end is fuzzy to me, it would be helpful with inline comments explaining what we are trying to achieve in order to review.

Sure I'll add comments in detail.

The fact we have an conditional treatment on our test data before we act on it is not a good sign, ideally the data we use for testing is known ahead of time and we can just apply the function under test on it.

Whenever we need to bump the current version to 2 the replace above will fail, which is not what the test is trying to test.

I am wondering if a better approach here would be to just have a static json string here instead.

Why do we need to do this parsing manually ourselves here? Is there anything special that we need in this parsing that is no needed elsewhere?
I'm assuming this data is used in other parts of the system, and there would be logic we can reuse... now if there is an issue with the parsing of jsonmaps we have to fix in 2 places...

I'm also wondering if asserting on a substring is the most robust way here... The substring might be there but other things could make the json parsing fail... What we want to make sure is that after the migration the json can be parsed without issues and that the enum values are correct.

We even have the migratedAsset object, so let's just assert that the processors are set correctly.

This test is not verifying that the processor string can be parsed properly, which is what we want... we could still have a ";" separating them and those asserts would most likely pass.

@ekcoh do you know how we could get access to the parsed list of processors so we could assert on that instead? I could only find InputControl.processors or InputActionState.processors, but I don't know how to go from an InputActionAsset to InputControl or InputActionState

It seems InputAction.ResolveBindings end up calling InputBindingResolver.AddActionMap which then calls InstantiateWithParameters and that leads to the parsing and creation of processors, but I am not sure how we could call that from a test and then retrieve the list of processors to assert on that.

Good catch, since the API design of Input Actions is based on string you can put whatever into Processors. So this test needs to complemented with code resolving the actions and the only way to actually prove the processors is correctly parsed and applied I believe would be to enable the actions and do ReadValue on the action after being resolved. To drive that action you also likely need a fake device to drive it with e.g. 1.0 and then see that e.g. scale processor multiples by 10 or similar.

In CoreTests_Actions.cs there are various examples on how to achieve this, e.g. search for tests containing ReadValue.

This would have been trivial if the string based API design was not a thing and instead actions had a list of actual processors on them.

Hi @LeoUnity @ekcoh, our NameAndParameters.ParseMultiple() explicitly checks for both seperators , and ; between processors entries. Either way we get the same result however I agree with semantic issues from string only assertions.
Should we go for
var parsed = NameAndParameters.ParseMultiple(action.processors).ToList(); Assert.That(parsed.Count, Is.EqualTo(expectedCount)); ?

Wouldn't it just work to do this? Its simple and very little code

var legacyJson = ....
var modernJson = ....

var ported = InputActionAsset.FromJson(legacyJson).ToJson();
Assert.That(modernJson, Is.EqualTo(ported));

Ideally you would have compared InputActionAsset objects but unfortunately it's not supported :/ Hence full cycle allows you to avoid the details. If porting is successful I would expect them both to be equal. Or even

var legacyJson = ....
var modernJson = ....

var ported = InputActionAsset.FromJson(legacyJson).ToJson();
Assert.That(InputActionAsset.FromJson(modernJson).ToJson(), Is.EqualTo(ported));

Since that wouldn't break either if formatting is updated

Yes it should work, earlier the cause of regression was not considering ; separator in the migration logic it only splits , and keeps the ; unchanged caused the serialization to break. Since now as @LeoUnity suggested the robust approach to use the NameAndParameters the above FromJson and ported json assertion should do good I believe.

our NameAndParameters.ParseMultiple() explicitly checks for both seperators , and ;

This is the source of the regression we have today and we are fixing on this PR. The comma and semicolon are both used, but in different contexts.

Take a look at this example, this should allow you to assert on the parsed processors:

        var inputActionAsset = AssetDatabaseUtils.CreateAsset<InputActionAsset>();
        var actionMap = inputActionAsset.AddActionMap("Action Map");
        actionMap.AddAction("Action", InputActionType.Value, processors: "Custom(SomeEnum=10)").AddBinding("<Gamepad>/leftStick");
        
        actionMap.ResolveBindings();
        
        Assert.That(actionMap.m_State.processors[0].GetType(), Is.EqualTo(typeof(CustomProcessor)));
        Assert.That((actionMap.m_State.processors[0] as CustomProcessor).SomeEnum, Is.EqualTo(SomeEnum.OptionA));

I still do not see why this isn't moved to outer scope and reused over iterations instead of being allocated on every iteration? Am I missing something?

Nitpick: Why create a result variable here instead of just

return new NameAndParameters { .... };

Why is this function existing if it only forwards to NameAndParameters.SerializeMultiple? Seems redundant

Maybe keep the old name instead?