fix buggy test with $ref in older drafts

[karenetheridge]

all keywords adjacent to $ref have no effect in draft7 and earlier.

[karenetheridge]

I don't know how all your implementations were passing with this test! Maybe you have a bug?

[gregsdennis]

Sounds like we should have tests against this scenario if it should have failed.

[gregsdennis]

Can you add a test for the scenario that should fail?

[karenetheridge]

I don't understand what you're asking. The test as it is in the patch is the test that should fail.

[gregsdennis]

Looking into this more deeply, I don't understand why it should have failed. The $id has no impact on this test (in neither the original nor your change) because the $ref is changing the file name in the base URI anyway, so it doesn't matter whether $ref is a sibling of it.

The only way the $id could have an impact is if it specified a different folder, e.g. http://localhost:1234/subfolder/schema-remote-ref-ref-defs1.json. This would be ignored in the original case, but it would change the base URI in the second.

[karenetheridge]

When the $id is respected, the $ref resolves to "http://localhost:1234/ref-and-defs.json", which maps to a file in the resources/ directory. Without $id, the $ref resolves to "ref-and-defs.json" which doesn't exist (and such resolution is undefined; my implementation aborts evaluation and generates a result object with one single error.

[gregsdennis]

Okay, so you're saying that without the $id, the domain (locahost:1234) isn't defined, and so $ref can't resolve. That makes sense.

Now that you've corrected the schema to operate as the test cases expect, we still need one that ensures $ref can't resolve when it's a sibling of $id. This is what I'm asking you add.

[karenetheridge]

We already have tests for that, in ref.json.

My goal in this specific PR is to fix a test that should not ever have worked.

[karenetheridge]

There's probably more we can test in here, e.g. with an adjacent $id specifically, or an adjacent definitions, but I'll address that in a separate PR.

[gregsdennis]

We already have tests for that, in ref.json.

Can you link to where we already have it? The closest I found was this, but it wraps the $ref in an allOf.

My goal in this specific PR is to fix a test that should not ever have worked.

Yes, but in finding this test that should never have worked, you've identified two cases that should be covered: the case that this is attempting to cover and the case of why this should fail. They each need coverage.

[karenetheridge]

The failure case cannot be covered. There is no way of expressing a uri resolution failure in our current testing infrastructure.

The general case of sibling keywords to $ref being ignored is already covered. The $id keyword specifically is not, but I did say I would handle that in a separate PR.

If there are no objections to the changes I have made here, I'm going to merge this.

[gregsdennis]

$ref being unable to resolve is definitely a case that can be covered. I return an error message in the defined format. Validation fails.

[karenetheridge]

No it can't, because there's no way to express it with the current testing syntax.

If you can come up with something, then by all means.