Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Use URI for refreshService id #123

Open
wants to merge 3 commits into
base: gh-pages
Choose a base branch
from
Open

Use URI for refreshService id #123

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
36e4c82
Use URI for refreshService id
clehner Sep 14, 2021
f209ecd
Merge branch 'gh-pages' into cel/refresh-service-id-uri
clehner Feb 3, 2022
e411982
Deprecate URL test
clehner Feb 3, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions implementations/generate.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This now requires that id be a URL ...

Copy link
Member Author

@clehner clehner Feb 4, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

2022-02-04-165704_890x725_scrot

After

2022-02-04-165709_900x557_scrot

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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".

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:
2022-02-09-122538_1198x726_scrot

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:
2022-02-09-124511_1191x720_scrot

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

];

const invalidTests = [
Expand Down
5 changes: 2 additions & 3 deletions test/vc-data-model-1.0/21-refresh.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,9 +54,8 @@ describe('Refresh Service (optional)', function() {
}
});

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() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

... while this now requires that id be a URI

// test that the `id`'s value is a valid URI
const doc = await util.generate('example-011.jsonld', generatorOptions);
const services = [].concat(doc.refreshService);

Expand Down