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
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. To make sure it's not a latter we're reviewing and improving the tests affected by this change.
[x] ve.dm.MWReferenceModel test
[] [[ https://gerrit.wikimedia.org/g/mediawiki/extensions/Cite/+/3b796ee233bfa831140ea0f550adce657b355c57/tests/qunit/ve-cite/ve.dm.Transaction.test.js#13 | ve.dm.Transaction test ]]
[] ...
==== 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}
Apart from the glitch with double `\\` the latter looks more completed than the former. So we might assume that the merge is more correct there.
====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 the change in VE.
**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 the patch// it seems that neither of them is true when we assume that the `keyIndexes` is unintentionally incomplete. But also //with the patch// it's not true in our example.