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

Adding annotations and did some clean up. #73

Merged
merged 13 commits into from
Jun 10, 2024
Merged

Adding annotations and did some clean up. #73

merged 13 commits into from
Jun 10, 2024

Conversation

BigBlueHat
Copy link
Member

  • Explain why test is skipped.
  • Remove skipped test/req no longer in spec.
  • Enable createVp in context property test.
  • Link to text for section 1.3 Conformance tests.
  • Link to text for section 4.3 Identifiers tests.
  • Link to text for section 4.3 Types tests.
  • Add note about not testing 4.5 name/desc.
  • Link to text for section 4.6 Cred Subject tests.
  • Link to text for section 4.7 Issuers tests.
  • Link to text for section 4.8 Validity Period.
  • Link to text for section 4.9 Securing Mechanisms.
  • Link to text for section 4.10 Status.

@BigBlueHat
Copy link
Member Author

@aljones15 @tminard I've been working my way through the spec adding these annotations to the code. They should be mostly up-to-date.

However, I'm wanting to pivot off of this baseline and refactor this test suite code a bit. It's currently painful to navigate it as the entire list of requirements is in on big bucket... So, once this is in, I plan to refactor, then return to annotating and sketching out what tests are missing.

@BigBlueHat BigBlueHat force-pushed the annotate-and-cleanup branch 2 times, most recently from b220bc0 to 1dabb60 Compare June 6, 2024 17:57
aljones15
aljones15 reviewed Jun 6, 2024
View reviewed changes
package.json Outdated
@@ -45,7 +45,7 @@
"chai": "^4.3.6",
"klona": "^2.0.5",
"mocha": "^10.0.0",
"vc-test-suite-implementations": "github:w3c/vc-test-suite-implementations"
"vc-test-suite-implementations": "../vc-test-suite-implementations"
Copy link
Collaborator

Choose a reason for hiding this comment

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

this is odd, why are you installing implementations from a local dir?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry...yeah. That wasn't supposed to sneak in there. My bad.

aljones15
aljones15 reviewed Jun 6, 2024
View reviewed changes
tests/10-vcdm2.js
@@ -310,6 +338,7 @@ describe('Verifiable Credentials Data Model v2.0', function() {
await assert.rejects(endpoints.issue(require(
'./input/credential-issuer-name-extra-prop-en-fail.json')));
});
// TODO: remove. no longer present.
Copy link
Collaborator

Choose a reason for hiding this comment

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

looks like the title just changed: https://w3c.github.io/vc-data-model/#names-and-descriptions

aljones15
aljones15 reviewed Jun 6, 2024
View reviewed changes
tests/10-vcdm2.js
@@ -294,6 +321,7 @@ describe('Verifiable Credentials Data Model v2.0', function() {
await assert.rejects(endpoints.issue(require(
'./input/credential-issuer-object-id-not-url-fail.json')));
});
// TODO: remove. no longer present.
Copy link
Collaborator

Choose a reason for hiding this comment

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

the text of the normative statement just changed: https://w3c.github.io/vc-data-model/#names-and-descriptions

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks. Noted.

aljones15
aljones15 requested changes Jun 6, 2024
View reviewed changes
Copy link
Collaborator

@aljones15 aljones15 left a comment

Choose a reason for hiding this comment

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

looks like 2 title changes are needed. I do like how you added the google highlight links to each statement before getting to work on the actual test vectors.

@BigBlueHat BigBlueHat mentioned this pull request Jun 7, 2024
Merged
@BigBlueHat
Copy link
Member Author

@aljones15 if you're OK with this, I'd like to wait on doing the issuer.name and issuer.description test changes. I can make them part of #74 if you'd like--and likely move them into the currently skipped name and description test section. We can then discuss with others where else to run the name and description tests. Issuer seems like a good candidate (and they're already written), but we can certainly discuss testing them in credentialSubject as well (if/when present, of course).

But...I'd love to get this one merged before dealing with that larger move (and base it on #74's new groups).

@BigBlueHat BigBlueHat requested a review from aljones15 June 7, 2024 18:32
aljones15
aljones15 approved these changes Jun 7, 2024
View reviewed changes
Copy link
Collaborator

@aljones15 aljones15 left a comment

Choose a reason for hiding this comment

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

Approving as PR has a lot of really good improvements and comments in it =)

@BigBlueHat BigBlueHat merged commit 838a24b into main Jun 10, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aljones15 aljones15 aljones15 approved these changes

@tminard tminard Awaiting requested review from tminard

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@BigBlueHat @aljones15