==== Context
We saw failing tests in Cite triggered by the change made in [[ https://gerrit.wikimedia.org/r/c/VisualEditor/VisualEditor/+/1196452 | Maintain keyIndexes when rebuilding InternalList ( 1196452 ) ]]
See https://integration.wikimedia.org/ci/job/quibble-vendor-mysql-php81/45819/console
==== Steps
At this point it's not entirely clear if the tests fail due to their complex setup, or if there's a use case that we're breaking. So this task is about To make sure it's not a latter we're reviewing and improving the tests affected by this change.
[x] Improve failing ve.dm.MWReferenceModel test to narrow down the issue
[] Improve failing [[ https://gerrit.wikimedia.org/g/mediawiki/extensions/Cite/+/3b796ee233bfa831140ea0f550adce657b355c57/tests/qunit/ve-cite/ve.dm.Transaction.test.js#13 | ve.dm.Transaction test ]] test to narrow down the issue
- https://gerrit.wikimedia.org/r/c/1199001
- https://gerrit.wikimedia.org/r/c/1199283
- https://gerrit.wikimedia.org/r/c/1199828
[] Figure out why the test is failing and if it's necessary
==== Insights:
The failing `ve.dm.MWReferenceModel` test was setup in a quite complicated way. So I'm reducing that complexity to narrow it down on what really needs to be tests here.
It's a bit more complicated with the failing transaction test, but here's my understanding so far:
- The transaction test merges two documents and checks if the transactions created for that merge make sense
- Merging two documents will also trigger the merge of the InternalList of each document
- The later is done in `ve.dm.InternalList.merge()`
- In there the code checks for duplicate keys used in both lists and tries to suggest a solution on how to deal with these by providing a mapping
- Here the test fails because the ref that should get merged into the list get's a different `listkey` than expected
{F67965217}
- Note, that the `listKey` of the new ref is still changed as expected to `auto/1` nevertheless
- Also so InternalItem of the ref that should be merged into the doc is not merged anymore but left out
{F67965395}
====InternalList.merge() input
I can see different input for `ve.dm.InternalList.merge()` when the [[ https://gerrit.wikimedia.org/g/mediawiki/extensions/Cite/+/3b796ee233bfa831140ea0f550adce657b355c57/tests/qunit/ve-cite/ve.dm.Transaction.test.js#13 | ve.dm.Transaction test]] is executed with or without [[ https://gerrit.wikimedia.org/r/c/VisualEditor/VisualEditor/+/1196452 | 1196452]]. Especially when it comes to the `keyIndexes`.
**Without the change**
{F67970233}
**With the change**
{F67970292}
What's weird here is that there are keys with `mwReferences//` and with `mwReferences/` and also `mwReferences/null` seems weird.
( Fixed in https://gerrit.wikimedia.org/r/c/1199828 )
====Why is the internal item removed?
According to [[ https://gerrit.wikimedia.org/g/VisualEditor/VisualEditor/+/b70ee4a11dfb2a5db4befc350470c43170cf42fa/src/dm/ve.dm.InternalList.js#399 | a comment in the code ]] in `ve.dm.InternalList.merge()` checks for duplicate keys and discards the other `InternalItem` when duplicates are found. The linked references will be re-mapped to the duplicate in the original. This also means that the content of the reference that gets merged in will be lost. With that in mind the behavior might be more correct with [[ https://gerrit.wikimedia.org/r/c/VisualEditor/VisualEditor/+/1196452 | 1196452]].
**OTOH:**
There's [[ https://gerrit.wikimedia.org/g/VisualEditor/VisualEditor/+/b70ee4a11dfb2a5db4befc350470c43170cf42fa/src/dm/ve.dm.TransactionBuilder.js#105 | another comment in the code ]] in `ve.dm.TransactionBuilder.newFromDocumentInsertion()` that states:
> any differences between internal items that doc and newDoc have in common will be resolved in newDoc's favor
So both comments contradict each other and //without [[ https://gerrit.wikimedia.org/r/c/VisualEditor/VisualEditor/+/1196452 | 1196452]]// it seems that neither of them is true when we assume that the `keyIndexes` is unintentionally incomplete.
But also //with [[ https://gerrit.wikimedia.org/r/c/VisualEditor/VisualEditor/+/1196452 | 1196452]]// only the former comment is true but not the latter.
====What's the test `newFromDocumentInsertion` testing?
`ve.dm.TransactionBuilder.static.newFromDocumentInsertion` is mainly used from the `ve.ce.ClipboardHandler` and `ve.dm.VisualDiff`. In both cases it seems that the input documents have populated `InternalItems` data. So the `keyIndexes` on the `InternalList` seem to be initially intact in either case.
**`ve.ce.ClipboardHandler`**
- `newFromDocumentInsertion` is called in `afterPasteInsertExternalData` to merge the data.
- Before `afterPasteInsertExternalData` is called it's made sure that `listKeys` don't conflict by calling `ve.dm.LinearData.prototype.remapInternalListKeys`
- So in this use case we will not run into conflicting `listKeys` it seems and testing the correct remapping of these in `newFromDocumentInsertion` should not be a valid use case
**`ve.dm.VisualDiff`**
- `newFromDocumentInsertion` is called to get the changes to the internal list between the old and the new document
See https://gerrit.wikimedia.org/g/VisualEditor/VisualEditor/+/b70ee4a11dfb2a5db4befc350470c43170cf42fa/src/dm/ve.dm.VisualDiff.js#39
- I'm not entirely sure what transitions are expected here to show a correct diff. In any case there should be other tests checking that I guess.
==== Cloned `InternalLists` vs tests
In all of the above cases `newFromDocumentInsertion` is called in places where the inputs were at least once cloned from a document or from data. So in either case, the `InternalList` does not have any information about `keyIndexes` because it's not copied.
So `ve.dm.InternalList.merge()` would never run into issues with conflicting `listKeys`. Therefore testing `newFromDocumentInsertion` with populated InternalItems and `keyIndexes` seems to be something that's not covering any real world cases.
But [[ https://gerrit.wikimedia.org/r/c/VisualEditor/VisualEditor/+/1196452 | Maintain keyIndexes when rebuilding InternalList ( 1196452 ) ]] changes the above assumptions. So now it's made sure, that the `InternalList` is always intact with `keyIndexes`. Conflicts between `listKeys` would now be handled during merge and might lead to different and unexpected outcomes.
This should still not be an issue in `ve.ce.ClipboardHandler`. As already mentioned `ve.dm.LinearData.prototype.remapInternalListKeys` make sure that there are no conflicts when merging.
So the remaining open question is, how the new code affects `ve.dm.VisualDiff`.