Allow custom assertions in XQSuite

[line-o]

Description:

Add new functions test:fail#3 and test:fail#4 to XQSuite. Calling one of these inside a XQSuite test function will stop execution by throwing a special error test:failure. This error is caught and handled as a failure allowing to create arbitrary custom assertions and the ability to provide specific error messages.

(edit) After @adamretter's feedback the order of arguments was changed.

• test:fail#3: fail with custom message, expected and actual value
• test:fail#4: fail with custom message, expected and actual value and a custom test type

Works in both XQuery and jUnit context.
In jUnit you will only be presented with the serialized expected and actual values.

(edit) In order to provide this facility also to packages that target older versions of eXist-db, which might not have the test:fail function available both

• the QName of the error that is treated as a failure $test:FAILURE
• the default custom assertion type $test:CUSTOM_ASSERTION_FAILURE_TYPE

are exposed. This allows to fall back to throwing the error directly in custom assertion code. In case of a failed assertion the test case will still fail. Though not as a failure but an error. Also expected and actual value might not be displayed nicely. But the provided custom message will still add value.

Example

test.xqm

xquery version "3.1";

module namespace t="http://exist-db.org/xquery/test/xqsuite";

import module namespace assert="http://line-o.de/xq/assert" at "./assert.xqm";

declare namespace test="http://exist-db.org/xquery/xqsuite";

declare variable $t:var := map {"a": 1, "b": 2};

declare %test:assertTrue function t:test-missing-key() as xs:boolean? {
    assert:map($t:var, map {"a": 1, "c": 4})
};

declare %test:assertTrue function t:test-wrong-value() as xs:boolean? {
    assert:map($t:var, map {"a": 1, "b": 3})
};

declare %test:assertTrue function t:test-wrong-type() as xs:boolean? {
    assert:map($t:var, [1,2])
};

declare %test:assertTrue function t:test-passing() as xs:boolean? {
    assert:map($t:var, $t:var)
};

assert.xqm

xquery version "3.1";

module namespace assert="http://line-o.de/xq/assert";

import module namespace test="http://exist-db.org/xquery/xqsuite"
    at "resource:org/exist/xquery/lib/xqsuite/xqsuite.xql";

declare function assert:map ($e as map(*), $a as item()*) as xs:boolean? {
    if (exists($a) and count($a) eq 1  and $a instance of map(*))
    then (
        for-each(map:keys($e), function ($key as xs:anyAtomicType) {
            if (not(map:contains($a, $key)))
            then test:fail("Key " || $key || " is missing", $e, $a, "map-assertion")
            else if ($e($key) ne $a($key))
            then test:fail("Value mismatch for key '" || $key || "'", $e, $a, "map-assertion")
            else ()
        })
        ,
        true()
    )
    else test:fail("Type mismatch", $e, $a, "map-assertion")
};

Running the above will yield

<testsuites>
    <testsuite package="http://exist-db.org/xquery/test/xqsuite" timestamp="2022-04-12T14:24:15.72+02:00" 
        tests="4" failures="3" errors="0" pending="0" time="PT0.008S">
        <testcase name="test-missing-key" class="t:test-missing-key">
            <failure message="Key 'b' is missing" type="map-assertion"/>
            <output>map{"a":1,"c":4}</output>
        </testcase>
        <testcase name="test-passing" class="t:test-passing"/>
        <testcase name="test-wrong-type" class="t:test-wrong-type">
            <failure message="Type mismatch" type="map-assertion"/>
            <output>[1,2]</output>
        </testcase>
        <testcase name="test-wrong-value" class="t:test-wrong-value">
            <failure message="Value mismatch for key 'b'" type="map-assertion"/>
            <output>map{"a":1,"b":3}</output>
        </testcase>
    </testsuite>
</testsuites>

Reference:

This was added due to the limitations of XQuery function annotations and will allow to compare complex values,
loading fixtures and assertions from modules and provide human readable feedback via custom error messages.

Type of tests:

XQSuite tests in custom-assertion.xqm

[adamretter]

I have a concern that your test:fail function is mixing concerns, it appears to do two things:

1. check that expected and actual match
2. log a failure message (conditionally if (1) is false)

The problem with mixing concerns is that we are then not creating composable building blocks.

I think it needs to be simplified to do one thing, i.e. fail with an optional code and message. For a good example, see the JUnit fail function, it does a single thing. fail is certainly not meant to be an assertion!

[line-o]

test:fail does not assert anything @adamretter
As you can clearly see in the provided example.

[adamretter]

test:fail does not assert anything @adamretter
As you can clearly see in the provided example.

The example isn't clear at all as it uses an assert library which isn't part of this PR. Instead I was reading the commits themselves...

[adamretter]

Ah I see... It doesn't assert, however including them as args to test:fail, still mixes concerns and by extension limits the usefulness of test:fail.

When I have a moment later I will try and propose some alternative function signatures, which still allow you to achieve the same goal but are more composable

[line-o]

In order to get the desired test output on failure it is crucial to pass both expected and actual to test:fail.
Function annotations are not sufficient to handle these.

[adamretter]

Thanks Juri. I understand and I am generally in favour of this, I think it just needs a little tweaking and discussion

[line-o]

@adamretter I am all for discussions. I would appreciate it, though, that you state what you think needs to change and why.
From reading through your comments, I understand that passing expected and actual to test:fail is what you dislike. I already pointed out why this needs to happen.
So the question remains how do you envision to pass those two values to the error that is thrown or more importantly the error handling code?

[adamretter]

I think the full function signature for a test:fail function should look like:

test:fail($code as xs:QName?, $description as xs:string, $failure-object as item()*) as none

We can then also have:

1. test:fail()
2. test:fail($code as xs:QName?)
3. test:fail($code as xs:QName?, $description as xs:string)

I have chosen the above function signatures to mirror that of fn:error, as failure is a parallel concept to error, and @line-o's proposed implementation uses fn:error itself.

By not mixing concerns, i.e. not forcing the user to include the expected and actual values of an assertion, the test:fail function now does one thing and one thing only.
There are of course also many use case, where you may want to fail when there is no assertion to be made; JUnit's fail function demonstrates that! The user is still free to provide any additional values they need (e.g. expected and actual values) in the $failure-object sequence.

A new implementation of such a function might look like:

declare function test:fail($code as xs:QName?, $description as xs:string, $failure-object as item()*) as empty-sequence() {
    fn:error(xs:QName("test:fail"), $description, ($code, $failure-object))
};

[line-o]

@adamretter I incorporated your feedback.

[sonarqubecloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
No Duplication information

[dizzzz]

Not completely my cup of tea, but great to see the interaction

[adamretter]

@line-o You still appear to have the old versions of test:fail which mix concerns (i.e. they still expect actual and expected values). Can you remove those please as they are now unnecessary.

[line-o]

@adamretter I cannot follow. Both expected and actual are necessary in every case.

[adamretter]

@line-o The premise behind all of my concerns with this PR is that having actual and expected values as explicit arguments mixes concerns in a negative way!

Instead, with my proposal you have $failure-objects, and you can simply do this instead (if you wish), which is equivalent but doesn't mix concerns:

test:fail($code, $description, ($expected, $actual))

[line-o]

Your proposal, @adamretter, will not work.
Here are the main issues:

• both the actual and expected values can be sequences and will be mashed together into a single sequence making it impossible for later code to distinguish them
• the later code needs to handle missing values (throwing an error in the error handler?)
• assertion developers need to keep in mind what and how is to be put there and no tooling will help them remember that

[adamretter]

both the actual and expected values can be sequences and will be mashed together into a single sequence making it impossible for later code to distinguish them

That is solved simply by using a Map, e.g.:

test:fail($code, $description, map {"expected" : $expected, "actual": $actual})

the later code needs to handle missing values (throwing an error in the error handler?)

I am not sure what you mean by "the later code" - can you show me where to find this?

assertion developers need to keep in mind what and how is to be put there and no tooling will help them remember that

Again I am afraid, I am not sure what you mean by this. Can you show me where to find this?
Also, keep in mind that not "all assertions" involve comparing actual and expected values, or even comparing values. The Hamcrest library can show you some good examples of other assertions that don't involve values.

[line-o]

@adamretter I am sure you noticed that test:fail#0, and test:fail#1 were added to this PR along with their tests.
So, if an assertion does not need to expected and actual vaiue it is as easy as:

declare %test:assertTrue function t:random-is-odd() as xs:boolean? {
    if (( random-number-generator()?number * 1e16 ) mod 2)
    then test:fail()
    else true()
};

Regarding the usage of a map. Yes, this would solve the problem of keeping sequences apart. On the flip side there is no tooling in place to help you keep in mind what keys need to propagated. Whereas having them as explicit arguments means that in code editors like eXide, VSCode and intelliJ you will be presented with useful hints what needs to be added.
And if one does not use those the XQuery compiler of existdb will throw a static error with the line number and column number where the argument is missing.

The "later code" is here

exist/exist-core/src/main/resources/org/exist/xquery/lib/xqsuite/xqsuite.xql

Lines 425 to 446 in 8464ae4

	let $serialized-expected := serialize($err:value?expected, map {"method": "adaptive"})
	let $serialized-actual := serialize($err:value?actual, map {"method": "adaptive"})
	return (
	if (not(empty($test-failure-function))) then
	$test-failure-function(
	test:get-test-name($meta),
	(: expected :)
	map { "value": $serialized-expected },
	(: actual :)
	map { "result": $serialized-actual }
	)
	else ()
	,
	test:print-result(
	$meta,
	$serialized-actual,
	<report>
	<failure message="{ $err:description }" type="{$err:value?type}" />
	<output>{ $serialized-actual }</output>
	</report>
	)

[line-o]

While one can leave the values empty, it will result in following output in jUnit

[dizzzz]

#confused

[line-o]

I believe two key purposes of a testing framework got confused here:

• assertions
• reporting

While the former has to have the expected and actual value the latter needs to know these only if the reporter wants to display these values. A dot-reporter for example only needs to know if the test passed or failed.

But all the reporters of XQSuite I am aware of - XML, HTML, jUnit - do display at least one of them.

That is why I chose to require them in the design of test:fail instead of making them optional.
It is also impossible for the test function to know which reporter is currently used.

I have a hard time thinking of a reason why this is a bad decision and have not read a good one in the comments, yet.

[line-o]

The only point I can think of is that it would be one parameter less, three instead of four.

Developer experience and help from tooling outweighs this argument by a ton in my opinion.

[adamretter]

I think there are some things here that still need to be discussed and developed further, I will try and find some time later in the week to take a look

[dariok]

As I read the discussion, the feature in itself is not in question but rather points related to usage?
I agree that this needs to be well-discussed as we should not release a feature people will rely on with the “threat” of major changes in the near future – especially not as this would require people to rewrite their tests.

Can we compile a list of concerns and possible pitfalls that should be discussed? That may be a point for an upcoming call’s agenda.

[line-o]

@adamretter can you elaborate on why you are against merging this PR? It's been several months.

[duncdrum]

@dariok merging into the develop branch is not the same as releasing a feature. The develop branch allows developers and users to test features and provide feedback together with everything else that's happening in exist. The risk of changes to features currently on the develop branch is well documented and communicated.

If further changes are necessary before a release, then that's a separate question. The question at hand is, if the PR is ready to be merged into develop. As it is not a breaking change, I d say yes. Further development of this or other features before or after the next release should happen in a new pr.

[dizzzz]

+1 for me pulling in; maybe not perfect but good enough

[sonarqubecloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
No Duplication information

[line-o]

needs to be rebases on current develop

[adamretter]

After a rebase, this is still awaiting further discussion and then likely some adjustment, before it gets merged

[sonarqubecloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
No Duplication information

[line-o]

@dizzzz I will rebase this branch

[duncdrum]

still in favour of pulling in, and seeing how the dice fall

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud