Please have a look at the test failures.

Please have a look at the test failures

@line-o All tests are now passing.

What about the error in Deploy / Build and Test Images?

What about the error in Deploy / Build and Test Images?

@line-o I don't know what the Bats tests do and I don't understand them. Any idea who added them and/or what they do?

They test the docker container. In this case https://github.com/eXist-db/exist/actions/runs/3574037174/jobs/6008812699#step:11:12 if the logs are free of errors on startup. They are not in this branch

Okay thanks @duncdrum, do you know where I can see the logs?

@adamretter simply build and run the container and see what it pipes to console, or download exist.log

@duncdrum Unless I am mistaken the exist.log or Docker console log is not available from the CI for this?

I built a Docker image locally, and it does indeed show that there was a hidden XQuery error in Monex which is shown by the fixes in this PR:

30 Nov 2022 22:03:45,321 [main] ERROR (AutoDeploymentTrigger.java [execute]:127) - Exception during deployment of app monex-3.0.4.xar: Error found while processing repo.xml: err:XPTY0004 xs:string(/db/apps/monex/data) is not a sub-type of xs:anyURI [at line 12, column 15, source: /exist/etc/../data/expathrepo/monex-3.0.4/post-install.xql]
org.expath.pkg.repo.PackageException: Error found while processing repo.xml: err:XPTY0004 xs:string(/db/apps/monex/data) is not a sub-type of xs:anyURI [at line 12, column 15, source: /exist/etc/../data/expathrepo/monex-3.0.4/post-install.xql]
	at org.exist.repo.Deployment.deploy(Deployment.java:469) ~[exist.uber.jar:6.1.0-SNAPSHOT]
	at org.exist.repo.Deployment.installAndDeploy(Deployment.java:281) ~[exist.uber.jar:6.1.0-SNAPSHOT]

Seems we will also need to fix Monex and get a new release of that out...

@line-o @duncdrum Okay I fixed this issue in a quick hotfix release of Monex 3.0.5 and this has fixed the build here.

related to #4632

Regarding problems with bundled applications: the necessity of commit a1a35e0 indicates that there might be more problems after this PR is merged.

This PR contains several - welcome and good - changes that would warrant their own separate PRs. The risk is that we accidentally create more harm than we solve.

Since one of the bundled applications seems to choke on the changes in this PR we need to investigate first if other packages might also be affected.

@line-o I am afraid that is not specific to this PR. That is just a general statement that: whenever we modify eXist-db's XQuery engine to be more spec compliant, then it is likely that any code that relied on behaviour that is not spec compliant will need to be fixed. This has been true for every XQuery change to eXist-db, and does not relate specifically to this PR.

This change, just like any other PR that changes eXist-db's XQuery compliance in a non-compatible way, is scheduled for a major release.

There is still an open question on the adaptive serialization test where the expected value changed from a string to a number literal.

@line-o As I tried to explain above there is a sequence of operations that happens. This test is not only testing that a specific string is returned from fn:serialize for Adaptive Serialization, but it is also testing the steps that happen after that. I will elaborate here to try and make the intention of the test clearer...

1. A Literal un-quoted annotation with value 1234 is passed as the argument to the function, and XQSuite has to parse this and cast it to the correct sub-type of xs:anyAtomicType. In this test it will be parsed and cast to xs:integer.
2. The function under test is executed by XQSuite.
3. The function under test eventually calls fn:serialize with the Adaptive Serialization mode set; fn:serialize returns an xs:string.
4. The function under test returns the xs:string value.
5. XQSuite needs to convert that xs:string value to a type and/or value that is appropriate for comparison to the expected value instructed by the %test:assert* annotation.
6. For this test the assert annotation has an *un-quotedvalue of1234. XQSuite has to coerce this and the result of the function to a type that it can compare according to the XQuery specs for type promotion and type derivation. In this test it will be parsed and cast to xs:integer`.
7. Finally the two xs:integer values are compared for equality.

I hope that helps explain the purpose of this test (which I think is the thing that is not being understood here). To summarise - it covers both the xs:string case that you are concerned about, as well as the actual xs:integer case that I am trying to test.

I would like to have proof that XQsuite is able to assert a string '1234' was returned.

That is already present on that test, both implicitly through the explanation above that uses:

%test:args(1234)
%test:assertEquals(1234)

and also explicitly by the additional assertions on the same function under test:

%test:args('Hello "world"!')
%test:assertEquals('"Hello ""world""!"')

Additionally... I think you are already aware of many tests, including some written by yourself, within the full XQSuite of tests that already cover that use-case.

The two tests in question do test the raw output of serialize (they are part of the spec of our implementation of fn:serialize). The function under test is fn:serialize. Thus have to return xs:string correct?

declare
    %test:args(1234)
    %test:assertEquals(1234)
    %test:args('Hello "world"!')
    %test:assertEquals('"Hello ""world""!"')
function ser:adaptive-simple-atomic($atomic as xs:anyAtomicType) {
    ser:adaptive($atomic)
};

declare
    %test:args(1234)
    %test:assertEquals(1234)
    %test:args('Hello "world"!')
    %test:assertEquals('"Hello ""world""!"')
function ser:adaptive-simple-atomic-map-params($atomic as xs:anyAtomicType) {
    ser:adaptive-map-params($atomic)
};

Thus have to return xs:string correct?

I am afraid @line-o that you are still missing the point of my changes. I am not sure how I can explain them any better, as I have tried several times now. Perhaps someone else could assist here?

So... Yes, fn:serialize returns an xs:string. I am not disputing that! I even said above:

3. The function under test eventually calls fn:serialize with the Adaptive Serialization mode set; fn:serialize returns an xs:string.

However, that's not the point of these additions that I have made. As I said above:

To summarise - it covers both the xs:string case that you are concerned about, as well as the actual xs:integer case that I am trying to test.

Can we please move on?

actual xs:integer case that I am trying to test.

The test are for fn:serialize! They must stay testing that the return value is xs:string, otherwise they do not make sense for anyone looking at them in a few months. They were changed in this PR for no apparent reason. Please let's move on by reverting the changes to those two tests.
If you want test type promotion capabilities of XQSuite, please add them separately to an xqsuite specific testsuite.

@dizzzz @reinhapa Would you be able to spare some time to help @line-o understand what I am trying to achieve here please?

actual xs:integer case that I am trying to test.

The test are for fn:serialize! They must stay testing that the return value is xs:string, otherwise they do not make sense for anyone looking at them in a few months. They were changed in this PR for no apparent reason. Please let's move on by reverting the changes to those two tests. If you want test type promotion capabilities of XQSuite, please add them separately to an xqsuite specific testsuite.

@line-o I can not see a problem with the changes from @adamretter here. In my opinion the fix is as much as important as the test. As I understand it your requirements where covered as explained by Adam earlier. I see no point of changing the tests again.

@reinhapa a test that has to ensure the function under test returns the expected value and type.
Since the modified tests test fn:serialize it has to be xs:string. At the moment this is the case. In this PR the expected return type is changed to a literal xs:decimal ("1234" was changed to 1234).
If this does not make a difference technologically it does change the meaning of the test semantically.
In worst case it can lead to uncaught regressions.

SonarCloud Quality Gate failed.   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
2 Code Smells

77.8% Coverage
0.0% Duplication