apple / pkl

A configuration as code language with rich validation and tooling.
https://pkl-lang.org
Apache License 2.0
9.85k stars 258 forks source link

Unable to create test for non-empty constraint on listing #461

Open jsundin opened 2 months ago

jsundin commented 2 months ago

The intention here is to create a test for checking a non-empty constraint on a listing, for regression purposes. I realize that pkl test has not been documented, so it's quite possible there are some misunderstandings or unsupported functionality being used here.

We need three files for a demonstration.

# NonEmptyDef.pkl:
values: Listing<String>(!isEmpty)

# NonEmptyImpl.pkl:
amends "NonEmptyDef.pkl"

values {
}

# NonEmptyTest.pkl:
amends "pkl:test"
import  "pkl:test"
import "NonEmptyImpl.pkl" as confUnderTest

examples {
    ["basic-test"] {
        test.catch(() -> confUnderTest)
    }
}

We can clearly see that values defined in NonEmptyImpl is invalid, as the listing is required to not be empty. However, when running this test:

$ pkl test --overwrite NonEmptyTest.pkl 
module NonEmptyTest (file:///tmp/issues/NonEmptyTest.pkl, line 1)
  basic-test ✍️
  NonEmptyTest ❌
    Error:
        -- Pkl Error --
        Expected an exception, but none was thrown.

        7 | test.catch(() -> confUnderTest)
            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
        at NonEmptyTest#examples["basic-test"][#1] (file:///tmp/issues/NonEmptyTest.pkl, line 7)

The error seems to suggest that there is no evaluation error here. If that's the case, we should be able to try the inverse test, as demonstrated in NonEmptyTest-Inv.pkl:

amends "pkl:test"
import  "pkl:test"
import "NonEmptyImpl.pkl" as confUnderTest

examples {
    ["basic-test"] {
        confUnderTest
    }
}

This time we get the opposite error instead; an error was thrown and we did not catch it:

$ pkl test --overwrite NonEmptyTest-Inv.pkl 
module NonEmptyTest-Inv (file:///tmp/issues/NonEmptyTest-Inv.pkl, line 1)
  basic-test ✍️
  NonEmptyTest-Inv ❌
    Error:
        -- Pkl Error --
        Type constraint `!isEmpty` violated.
        Value: new Listing {}

        1 | values: Listing<String>(!isEmpty)
                                    ^^^^^^^^
        at NonEmptyDef#values (file:///tmp/issues/NonEmptyDef.pkl, line 1)

        3 | values {
            ^^^^^^^^
        at NonEmptyImpl#values (file:///tmp/issues/NonEmptyImpl.pkl, line 3)

If we instead embed this list inside a mapping, this works as expected:

# NonEmptyDef2.pkl:
stuff: Mapping<String, Listing<String>(!isEmpty)>

# NonEmptyImpl2.pkl:
amends "NonEmptyDef2.pkl"

stuff {
    ["key"] {
    }
}

# NonEmptyTest2.pkl:
amends "pkl:test"
import  "pkl:test"
import "NonEmptyTest2.pkl" as confUnderTest

examples {
    ["basic-test"] {
        test.catch(() -> confUnderTest.stuff)
    }
}
$ pkl test --overwrite NonEmptyTest2.pkl
module NonEmptyTest2 (file:///tmp/issues/NonEmptyTest2.pkl, line 1)
  basic-test ✍️

$ cat NonEmptyTest2.pkl-expected.pcf 
examples {
  ["basic-test"] {
    "Type constraint `!isEmpty` violated. Value: new Listing {}"
  }
}

The above has been tested using the following versions:

translatenix commented 2 months ago

I think everything works as expected here. (NonEmptyTest-Inv.pkl isn’t the inverse test.) This should work:

examples {
    ["basic-test"] {
        test.catch(() -> confUnderTest.values) // or: confUnderTest.output.value
    }
}

Here is a more robust test that doesn’t depend on the exact error message:

facts {
    ["basic-test"] {
        test.catchOrNull(() -> confUnderTest.values) != null
    }
}
holzensp commented 2 months ago

The error seems to suggest that there is no evaluation error here.

This is because there isn't. Where you say confUnderTest, you just refer to the object; nothing's forcing the evaluation of its members. If, instead, you say confUnderTest.output.text or confUnderTest.values.length, you force the evaluation of values and it throws as you'd expect (so test.catch catches it and the test passes).

If we instead embed this list inside a mapping, this works as expected:

Actually; that's not as expected. This is a known bug. Mapping<K,V> is too eager; it should be strict in its keys and lazy in its values, but currently, type checking it forces its values. (See: https://github.com/apple/pkl/issues/406)