Enveloped proofs helper functions #114
Conversation
Signed-off-by: Patrick <[email protected]>
PatStLouis
changed the title
| './input/credential-ok.json')); | ||
| } catch(e) { | ||
| console.error( | ||
| `Issuer: ${name} failed to issue "credential-ok.json".`, |
There was a problem hiding this comment.
we should probably store this error and ensure it shows up in the report so they know that the issuance failed.
| 'for that property', | ||
| async function() { | ||
| this.test.link = `https://www.w3.org/TR/vc-data-model-2.0/#conformance:~:text=MUST%20include%20all%20required%20properties%20in%20the%20conforming%20documents%20it%20produces`; | ||
| this.test.cell.skipMessage = 'Tested by other tests in this suite.'; |
There was a problem hiding this comment.
this can be handled by awaiting a previous call, you can do something like this
async function testSomething(){
return endpoints.post('goo');
}
// store a promise here
const somethingTest = testSomething();
// await the promise
it('assertion 1', async function() {
await somethingTest;
})
// await the promise again
it('assertion 2', async function() {
await somethingTest;
})
There was a problem hiding this comment.
I wouldn't go that far. In fact, I realized when going back through the MUSTs just now that all of Section 1 Introduction (of which 1.3 Conformance is a part) is non-normative...so we can remove the 1-3-conformance.js file altogether (and should for maintainability).
Let's merge this PR first, though.
There was a problem hiding this comment.
Actually, since you've brought them all in the way you have, we could do what @aljones15 is suggesting. It'll make the code read a little strangely (all the test's will get called outside of any it() and then their results awaited in each it()), but if y'all feel this will comfort developers and spec reviewers who will see those MUSTs and wonder why they're not tested, I'm more than happy for these to stay.
Just still on the fence as to whether the await approach is worth the effort. Maybe let's discuss it soon.
| .include('data:application/vc+jwt', | ||
| `Expecting id field to be a data: URL [RFC2397].`); | ||
| const extractedCredential = extractIfEnveloped(issuedVc); | ||
| shouldBeCredential(extractedCredential); | ||
| this.skip(); |
There was a problem hiding this comment.
why do we assert here and then skip?
There was a problem hiding this comment.
@PatStLouis guessing the above this.skip() just got overlooked?
There was a problem hiding this comment.
The problem here is because this test will also be executed on a non enveloped credential. I put the skip there until we have some way to tag implementations, I should of left a comment!
| this.test.cell.skipMessage = 'TBD'; | ||
| issuedVc.should.have.property('type').that.does | ||
| .include('EnvelopedVerifiableCredential', | ||
| `Expecting type field to be EnvelopedVerifiableCredential`); | ||
| this.skip(); |
There was a problem hiding this comment.
this is odd we assert and then skip that seems off.
There was a problem hiding this comment.
same as above, currently skipped until we have a way to tag EnvelopedProof implementations as otherwise this test will always fail on other implementations
| 'for that property', | ||
| async function() { | ||
| this.test.link = `https://www.w3.org/TR/vc-data-model-2.0/#conformance:~:text=MUST%20include%20all%20required%20properties%20in%20the%20conforming%20documents%20it%20produces`; | ||
| this.test.cell.skipMessage = 'Tested by other tests in this suite.'; |
There was a problem hiding this comment.
Actually, since you've brought them all in the way you have, we could do what @aljones15 is suggesting. It'll make the code read a little strangely (all the test's will get called outside of any it() and then their results awaited in each it()), but if y'all feel this will comfort developers and spec reviewers who will see those MUSTs and wonder why they're not tested, I'm more than happy for these to stay.
Just still on the fence as to whether the await approach is worth the effort. Maybe let's discuss it soon.
|
@PatStLouis what are your thoughts about handling the 1.3 Conformance stuff. How you have it now is fine by me (and I'll update my review), but we actually don't need to even provide that 1.3 section...as it's non-normative. |
Discussed this more with Manu and think proceeding with what you have here would be best. |
|
@PatStLouis let's also remove the first (currently skipped) entry: That's rather the point of the whole test suite. 😁 |
Signed-off-by: Patrick <[email protected]>
Co-authored-by: Ted Thibodeau Jr <[email protected]>
Signed-off-by: Patrick <[email protected]>
Signed-off-by: Patrick <[email protected]>
Signed-off-by: Patrick <[email protected]>
|
once @TallTed approves the changes I will merge this, the location of the file changed (outdated) but I've implemented your suggestions! |
Moved securing mechanism tests and added enveloped related helper functions.
I think we need a tagging option for
EnvelopedProofvsEmbededProof.