Use URI for refreshService id #123
Conversation
|
One file now requires URI, the other now requires URL. These should be the same. (Whether they should both be URI or URL seems to be currently being debated in other issues.) |
|
@TallTed I'm not sure what you mean. Which files? |
| @@ -48,6 +48,7 @@ const deprecatedTests = [ | |||
| 'Basic Documents Presentations MUST include `verifiableCredential` and `proof` (negative - missing `proof`)', | |||
| 'Zero-Knowledge Proofs (optional) A verifiable presentation... MUST include `verifiableCredential`', | |||
| 'Zero-Knowledge Proofs (optional) A verifiable presentation... MUST include `verifiableCredential` (negative - missing `verifiableCredential`)', | |||
| 'Refresh Service (optional) each object within `refreshService`... value of `id` MUST be a URL identifying a service endpoint', | |||
There was a problem hiding this comment.
This now requires that id be a URL ...
There was a problem hiding this comment.
Oh, this is just the list of deprecated tests. I've updated the PR text now to mention this.
Adding a test to this list suppresses it from appearing the resulting implementation report web page. Otherwise it would still appear, since it is present in some of the implementation report JSON. files.
Edit: added screenshots below.
Before
After
The new test should appear after an implementation's JSON file is updated after re-running the tests such that at least one implementation reports a result for this test.
There was a problem hiding this comment.
Alternatively, the implementation files could be search-and-replace edited to rename the test. Maybe this would be okay, considering it's the same test, just with a different name?
There was a problem hiding this comment.
I don't understand why/how no implementations would report a result for a given test, whether that's "unsupported", "supported and successful", or "supported and failed" — unless the test didn't yet exist, which makes me wonder whether the feature being tested had been spec'ed.
If the feature hadn't been spec'ed at time of test run, the result should probably default to report "unsupported".
If the feature had been spec'ed but the test didn't exist when the implementation was tested, the result should probably default to some shorthand for "test had not been implemented when implementation was tested" — in which case, implementers should be strongly encouraged to run the updated test suite and submit an updated report!
There was a problem hiding this comment.
I had said
If the feature hadn't been spec'ed at time of test run, the result should probably default to report "unsupported".
Re-reading, that default should probably indicate "untested" instead of "unsupported", because being unspec'ed there would have been no test to run, and so no way to know whether supported or not. This would also apply in cases where "the feature had been spec'ed but the test didn't exist".
There was a problem hiding this comment.
An implementation can skip sections of the test suite using the "sectionsNotSupported" field in their config.json. Then the tests in that section get the result "no support".
The "untested" result happens occurs when the test was added (or renamed) after the particular implementation's report JSON was generated. Wwith the change in this PR, after at least one implementer re-runs the test and adds their report, all the other implementer columns will read "untested" for this test. e.g. after I re-run the test for Spruce:
If instead I add a dummy test report ("Foo" implementation) that skips the "refresh" section using sectionsNotSupported, Foo's column says "no support" since it skipped the section - as in the following:
Do you mean to suggest adding new result statuses, to distinguish between current possible different reasons for the "untested" result (and/or "no support")? I'm trying to understand what these would be, and why it may be needed.
There was a problem hiding this comment.
I'm good with most of what you showed here.
One major difference -- I think every test should have a row of results. If no implementation has reported a result for that test, all implementations should show "untested" for that test -- in no small part because that may be revealing a feature that is at risk because two successful implementations are required to exit CR! -- so this should lead to reruns by at least two implementations that include support for that feature.
| it('value of `id` MUST be a URL identifying a service endpoint', async function() { | ||
| // test that the `id`'s value is a valid URL | ||
| // (possibly) attempt to dereference the service endpoint to validate it's "locator-ness" (i.e. URL vs. URI) | ||
| it('value of `id` MUST be a URI identifying a service endpoint', async function() { |
There was a problem hiding this comment.
... while this now requires that id be a URI
|
The issue was discussed in a meeting on 2022-02-09
View the transcript4.1. [Tracking Issue - Proposed Corrections Feedback] Test suite improvements are needed (issue vc-test-suite#126)See github issue vc-test-suite#126. Charles Lehner: Tallted has commented on this.
See github pull request vc-test-suite#123. Charles Lehner: when a test has not been evaluated or when feature did not previously exist, I still need to respond to TallTed. See github pull request vc-test-suite#127. Brent Zundel: is there anyone on the call that can review these?. Manu Sporny: you can add me. |
Change test title to use URI instead of URL.
The old test is added to the list of deprecated tests so that it no longer appears in the results.
Corresponds to w3c/vc-data-model#819