-
Notifications
You must be signed in to change notification settings - Fork 20
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.
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
Data providers tests for vip3 #2431
Conversation
1b0342d
to
e51c132
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you explain how and why we are testing data provieders through typescript tests ?
Or maybe we are just testing request vc cli commands ? 🙃
I think there should be no thing like dataproviders in ts code, it introduces unnecessary coupling.
We indeed need real accounts and a real endpoint to test the value of VCs. To be precise, we are assisting IdHub in identifying issues with VCs—for example, cases where an real account should meet the conditions for generating a credential, but IdHub receives a false value. This has occurred before. How: We might need the CI to use the data-provider's key (expect VIP3).
CLI help creating an ID graph with a real account. It ensures that Alice meets the conditions for being an ETH holder, and when requesting a VC from the data-provider, it should return a value of true. cc @kziemianek |
8a94f6e
to
4a945ee
Compare
55a4961
to
f76265a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't read thoroughly, but I feel it's a bit overcomplicated in general.
Can't we have some testdata.json
which includes:
{
[
"idgraph": [
"did1",
"did2",
],
"assertion": "a1",
"expectedValue": true
],
[
],
...
}
The script just iterates through the testdata, construct idgraph and request vc to see if we get the expected result.
Isn't that more clear and simple?
Just my naive thought - I could be wrong
I can modify it into a JSON file; the effect will be the same. |
2791aa9
to
0411ac4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Checked only source code and later will try to run everything locally. Put some comments.
@Verin1005 let me know what do you think about it?
assert.equal(events.length, 1); | ||
} | ||
|
||
async function requestVc(id: string, index: number) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is it possible to utilise this function for other scenarios, like NFT holder or BNB domain?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're referring to other cases, right? Currently, this pr is for VIP3, but I am testing the other cases. To avoid ambiguity, I am redefining credentials array here.
} = new URL(process.env.WORKER_ENDPOINT!); | ||
const { protocol: nodeProtocal, hostname: nodeHostname, port: nodePort } = new URL(process.env.NODE_ENDPOINT!); | ||
|
||
async function linkIdentityViaCli(id: string) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is it possible to utilise this function for other VC scenarios, like NFT holder or BNB domain?
ddcd262
to
5da02c2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. Thanks @Verin1005 for initiating this big topic as it will definitely save us time for a long run
Context
resolves p-366
Usage example:
If you want to test a specific VC(
vip3-membership-card-gold
), you can simply input its ID on the command line, like:pnpm --filter integration-tests run test-data-provider:local --id=vip3-membership-card-gold
.(you can find the ID in vc jsonfileTesting parameters(i.e. mockAddress or expectedCredentialValue) can be directly modified in the *.json files.
I feel this approach is relatively maintainable. If it seems reasonable, I'll add the others as well. This PR currently provides four.
If you have a better solution, please feel free to share it at any time.