Revamped output testing

[gregsdennis]

Following up on #247 and in light of @jdesrosiers' #652, I think we can do better for output tests.

The current test structure presents a JSON Schema that constrains several locations to specific values.

Example:

JSON-Schema-Test-Suite/output-tests/draft2020-12/content/readOnly.json

Lines 13 to 33 in 19947ea

	"output": {
	"basic": {
	"$id": "https://json-schema.org/tests/content/draft2020-12/readOnly/0/tests/0/basic",
	"$ref": "/draft/2020-12/output/schema",
	"properties": {
	"annotations": {
	"contains": {
	"properties": {
	"keywordLocation": {"const": "/readOnly"},
	"absoluteKeywordLocation": {"const": "https://json-schema.org/tests/content/draft2020-12/readOnly/0#/readOnly"},
	"instanceLocation": {"const": ""},
	"annotation": {"const": true}
	},
	"required": ["keywordLocation", "instanceLocation", "annotation"]
	}
	},
	"errors": false
	},
	"required": ["annotations"]
	}
	}

However, maybe a simpler approach (both to implement and write) would be just to have a series of JSON Pointers and the values that are expected to be in the output at those locations. For example, the above test file could be re-written as:

[
    {
        "description": "readOnly generates its value as an annotation",
        "schema": {
            "$schema": "https://json-schema.org/draft/2020-12/schema",
            "$id": "https://json-schema.org/tests/content/draft2020-12/readOnly/0",
            "readOnly": true
        },
        "tests": [
            {
                "description": "readOnly is true",
                "data": 1,
                "output": {
                    "basic": {
                        "/annotations/0/keywordLocation": "/readOnly",
                        "/annotations/0/absoluteKeywordLocation": "https://json-schema.org/tests/content/draft2020-12/readOnly/0#/readOnly",
                        "/annotations/0/instanceLocation": "",
                        "/annotations/0/annotation": true
                    }
                }
            }
        ]
    }
]

As long as all of those locations in the output contain those values, it is considered valid.

There is one caveat that the current version of the test also enforces that the the errors property is disallowed when annotations is present, which is something that the spec requires, but the proposal can't cover that case. I'm open to options here if that's important to us.

Edit: I've since realized that the pointers need indices because annotations is an array. I'm not sure if we can specify that the first one needs to be a particular value. Items in the array could be in any order. Maybe the tests could be engineered so that it's the only expected item?

[jdesrosiers]

I put some thought into this and this was the best I could come up with.

[
  {
    "description": "readOnly generates its value as an annotation",
    "schema": {
      "$schema": "https://json-schema.org/draft/2020-12/schema",
      "$id": "https://json-schema.org/tests/content/draft2020-12/readOnly/0",
      "readOnly": true
    },
    "tests": [
      {
        "description": "readOnly is true",
        "data": 1,
        "assertions": [
          {
            "outputUnit": {
              "keywordLocation": "/readOnly",
              "absoluteKeywordLocation": "https://json-schema.org/tests/content/draft2020-12/readOnly/0#/readOnly",
              "instanceLocation": "",
              "annotation": true
            },
            "contains": true,
            "expectedLocations": {
              "flag": null,
              "basic": "/annotations",
              "detailed": "/annotations",
              "verbose": "/annotations"
            }
          }
        ]
      }
    ]
  }
]

Disregarding nesting, the output unit for is the same regardless of which output format is used. So, we should be able to say which output unit is expected and pair that with where it's expected to be depending on which format is used. The test would pass if the given "outputUnit" is present anywhere in the array pointed at by the "expectedLocation". An output unit would be considered present if all of the keywords in the given output unit are both required and the same value as output unit being compared.

A major challenge I see with these tests is that there isn't a way to say an outputUnit must not appear somewhere. We might need to do that for testing a conditional. To address that, I included an "contains" property. If true the output unit must be present at the location, if false it must not be present. If we think there's a possibility for other types of comparisons, that could be made into something more complex.

The biggest thing I notice that this can't express is if there are extra output units that are unexpected.

[jdesrosiers]

I wonder if the best thing to do is just to include the full expected output result for the evaluation. Test harnesses would have to write code to compare it with their output in a way that ignore missing properties if they're optional, ignore extra properties, and compare arrays without regard to order. That's some non-trivial code to have to write for a test, which isn't great. But, ultimately, that would probably be the best way to test that output is interoperable enough between implementations to be useful for third-party processing.

[gregsdennis]

One of the problems that @karenetheridge and I recognized early on was that error messages can't be tested. You need to test that message is present, but you can't test the content because that's not specified. Having a schema was how I addressed this.

That issue would exist with what you have unless we can somehow say that

{
  "keywordLocation": "/readOnly",
  "absoluteKeywordLocation": "https://json-schema.org/tests/content/draft2020-12/readOnly/0#/readOnly",
  "instanceLocation": "",
  "error": "some error message"
}

is just as acceptable as

{
  "keywordLocation": "/readOnly",
  "absoluteKeywordLocation": "https://json-schema.org/tests/content/draft2020-12/readOnly/0#/readOnly",
  "instanceLocation": "",
  "error": "a different error message"
}

I do like your general approach, though. contains needs some expansion, I think, to provide more assertion options. (Not sure if we're over-engineering this.)

[jdesrosiers]

I didn't think about that one. That's another thing is couldn't express.

Not sure if we're over-engineering this.

I don't thing there's a simple solution here. No matter what we do, it's going to be complex.

[gregsdennis]

Iterating on @jdesrosiers' idea:

[
  {
    "description": "readOnly generates its value as an annotation",
    "schema": {
      "$schema": "https://json-schema.org/draft/2020-12/schema",
      "$id": "https://json-schema.org/tests/content/draft2020-12/readOnly/0",
      "type": "integer",
      "readOnly": true
    },
    "tests": [
      {
        "description": "readOnly is true",
        "data": 1,
        "assertions": [
          {
            "valid": true,
            "keywordLocation": "/readOnly",
            "absoluteKeywordLocation": "https://json-schema.org/tests/content/draft2020-12/readOnly/0#/readOnly",
            "instanceLocation": "",
            "annotation": true,
            "operation": "contains",
            "expectedLocations": {
              "flag": null,
              "basic": "/annotations",
              "detailed": "/annotations",
              "verbose": "/annotations"
            }
          }
        ]
      },
      {
        "description": "annotation not collected",
        "data": "string",
        "assertions": [
          {
            "valid": false,
            "keywordLocation": "/type",
            "absoluteKeywordLocation": "https://json-schema.org/tests/content/draft2020-12/readOnly/0#/type",
            "instanceLocation": ""
            "hasError": true,
            "operation": "contains",
            "expectedLocations": {
              "flag": null,
              "basic": "/errors",
              "detailed": "/errors",
              "verbose": "/errors"
            }
          },
          {
            "keywordLocation": "/readOnly",
            "absoluteKeywordLocation": "https://json-schema.org/tests/content/draft2020-12/readOnly/0#/readOnly",
            "instanceLocation": "",
            "annotation": true,
            "operation": "not-contains",
            "expectedLocations": {
              "flag": null,
              "basic": "/annotations",
              "detailed": "/annotations",
              "verbose": "/annotations"
            }
          }
        ]
      }
    ]
  }
]

The contains is now just one of several operations we could define rather than a specific key. The second assertion in the failing test uses a not-contains (naming is hard) operation, meaning (here) that we don't want the annotation expressed in the output.

Instead of outputUnit as a JSON object to be matched, we list the keys that an output unit is supposed to have. We also have hasError to indicate whether a unit should have an error or not. Omitting a key means that any value could be there.

We can still put the required values in a nested object, but I advise against calling it outputUnit since that seems to imply that a JSON-equality match should be performed (which will likely fail because we're not checking the content of error messages and the spec allows for additional information).

Lastly, 2019-09 and 2020-12 don't require that absoluteKeywordLocation be present except when a $ref (or similar) has been followed. That would have to be built into the test harness. "If the output unit contains it, this is what the value must be."

[gregsdennis]

An alternate iteration is to just consider outputUnit as a "minimal match." By that I mean that the listed properties must be matched, but unlisted properties are also allowed. It's like a kind of polymorphic equality. We would need to call that out in the readme.

So if { "foo": 1, "bar": "string" } is required, then { "foo": 1 } would not match but { "foo": 1, "bar": "string", "baz": true } would match.

I also still think that contains: true should be changed to operation: contains.

[gregsdennis]

I've created a branch (see commit ☝️) in my implementation that does the "alternate iteration" just above. The equality code wasn't hard to write, but I did need to do something custom for it.

[gregsdennis]

Just toying with this again, and expanding on the "minimal match" (I think I like "subset equality" better), I came up with this:

[
    {
        "description": "readOnly generates its value as an annotation",
        "schema": {
            "$schema": "https://json-schema.org/draft/2020-12/schema",
            "$id": "https://json-schema.org/tests/content/draft2020-12/readOnly/0",
            "readOnly": true
        },
        "tests": [
            {
                "description": "readOnly is true",
                "data": 1,
                "output": {
                    "basic": {
                        "/annotations/#": {
                            "keywordLocation": "/readOnly",
                            "absoluteKeywordLocation": "https://json-schema.org/tests/content/draft2020-12/readOnly/0#/readOnly",
                            "instanceLocation": "",
                            "annotation": true
                        }
                    }
                }
            }
        ]
    }
]

In this case, we have a sort of pointer template where # is a stand-in for any number. This would also allow us to go multiple levels deep by using multiple stand-ins: /annotations/#/annotations/#

So some item in the /annotations array needs to match the object listed there, but it could have more properties.

---

Honestly, I think what we need is JSON Path, but I understand the issues of introducing another technology, especially one that doesn't have as wide support as JSON Schema.

With JSON Path, we could easily write $.annotations[*] and check that the object is in the returned set.