-
Notifications
You must be signed in to change notification settings - Fork 72
ipAsset integration tests - edge and complex cases
#417
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
Conversation
| let parentIpId: Hex; | ||
|
|
||
| before(async () => { | ||
| client = getStoryClient(); |
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.
isn't this already created in the outer describe?
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.
No. I needed to reset the scope to force some errors.
| const res = await client.license.registerNonComSocialRemixingPIL({ | ||
| txOptions: { waitForTransaction: true }, | ||
| }); | ||
| noCommercialLicenseTermsId = res.licenseTermsId!; |
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.
we should try to reuse items like this and ips when possible to reduce the run time of our integration tests.
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.
Ideally, we should try, but in some cases, since we are trying multiple configurations, they need to be reset anyway. In other cases, they are out of scope.
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.
in this case, shouldn't noCommercialLicenseTermsId be the same ID as the one you have in the outer scope? Since the terms are the same, the POC should return the same license id i believe. So we can avoid doing multical rpc calls here just to fetch the same id.
| tokenId: tokenId!, | ||
| txOptions: { waitForTransaction: true }, | ||
| }); | ||
| parentIpId = parentIpResponse.ipId!; |
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.
we can probably reuse the parent ip created earlier
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.
The previous parentIpId is out of scope, i.e., not in the same describe
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.
we do have a parentIpId in the most outer scope IP Asset Functions. I understand if we need a brand new IP with a clean state. But if we don't or can avoid it, then lets try to reuse.
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.
Tests look great. my comments on reducing RPC usage if possible is not blocking but a general item we should be considering whenever we add more tests.
Description
This PR adds edge cases and more complex tests to the
ipAssetmodule integration tests.