[test] extend tests for immutability of maps

[line-o]

Description:

This PR now only adds tests that extend the previous test for the immutability of maps

fork maps before mutating them

Reference:

refs #3724

Map implementation (bifurcan) documentation of the forked() method
https://lacuna.io/docs/bifurcan/io/lacuna/bifurcan/IMap.html#forked--

Type of tests:

Extended XQsuite tests in exist-core/src/test/xquery/maps/maps.xql

[line-o]

So, in SingleKeyMapType#put a different method is used to create the copy of the map.
return new MapType(context, map.forked(), keyType);
I have not enough knowledge yet which one is the better in the instances changed here.

.forked() or .clone() - this will likely have effects on memory usage and maybe performance.

[line-o]

I do not understand the underlying implementation of "forked", though:

    default IMap<K, V> forked() {
        return this;
    }

[line-o]

using .forked() also works, so we can choose whichever is best. I tend to go with the forked method.

[sonarqubecloud]

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

[line-o]

Ready to be merged from my point of view. Tests fail until the fix is applied.

[joewiz]

I can confirm that when building eXist from this PR's branch, the tests that @PieterLamers and I developed in #3724 all return the expected results.

[line-o]

@joewiz great!

Do you think the test cases I added are covering every combination we need to check?
https://github.com/eXist-db/exist/pull/3725/files#diff-6cbf91fe2ed15b2f66e0c96ba43b21e94f3be9075de054b215a1cb0b5a88cfd7

[joewiz]

@line-o Yes, I think you've got the ones in the issue covered with mt:immutable-put-after-merge, mt:immutable-remove-after-merge, and mt:immutable-merge-after-merge, and the other variations were a great idea too.

[adamretter]

@line-o I just built the HEAD of Bifurcan and checked against that too. I wanted to do so, as it has some fixes that are not released yet. Unfortunately there was no change, still 6 failing tests. I will dig a little deeper...

[adamretter]

@line-o I just expanded on some of your tests, and it caused me to find another bug in maps. The XQuery F+O specs says that default behaviour for map:merge when there are duplicates keys is to use-first, however eXist-db does use-last and some of your tests were relying on that - I will fix that too...

[dizzzz]

Is there any new info on this?

[dizzzz]

what is the state of the PR? @line-o @adamretter
I marked it as draft to prevent accidental pulling in

[line-o]

I am waiting for this to either a) be merged as it is or for a review that tells what needs to change (I expect this to be "remove the surplus calls to forked()")
What I will definitely do is fix the minor issue in one the tests.

[line-o]

I can confirm that #3724 is fixed by the new release of bifurcan.

[joewiz]

@line-o That's great news! Thank you for all of your efforts on the maps issue, Juri! When you have the chance, could you please approve #3886?

[line-o]

Oh wait. I was under the impression that this PR was already applied!
This needs to be retested.

[line-o]

Rebase and with improved test cases.
I still see odd behaviour and fear that there is a) still issues in the map implementation or b) issues in how this library is used.

[sonarqubecloud]

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

0.0% Coverage
0.0% Duplication

[adamretter]

This is no longer needed, as the bug was indeed in Bifurcan and was fixed as lacuna/bifurcan#35 (comment); upgraded in eXist-db as #3886

The tests from this PR were incorporated and expanded upon in #3738

@line-o are you happy to close this one now?

[line-o]

@adamretter I actually re-incorporated the tests here and added a final improvement to them. What about we make this PR the one adding the tests and #3738 can then be solely about map:merge#2.
That is also nicer later when we compile the changelog.

[line-o]

I will take responsibility in changing the PR description and title as well as the commit history assuming you do so for #3738 as well.

[adamretter]

@line-o I don't think that makes sense, you would also need to remove all your Java changes in this PR;
As they were an early attempt to fix the Maps problem that was reported in eXist-db's code, but the real problem wasn't in eXist-db's code... it was in Bifurcan which I worked on to diagnose and get it resolved at its source. Wouldn't this PR then solely end up just being tests most of which were created/expanded/refactored in my other PR?

If you want my PR split into two, with and without the change to merge-order to match the spec, I have previously already offered to do that...

[line-o]

Of course I will remove the no longer needed commits.

[line-o]

I would also rather squash all the back and forth in the tests into a single commit.

[sonarqubecloud]

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
No Duplication information