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.

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

P-397 Add the new VC test case for ts-tests. #2537

Merged
merged 17 commits into from
Mar 5, 2024
Merged

Conversation

0xverin
Copy link
Contributor

@0xverin 0xverin commented Feb 29, 2024

Context

resolves p-397

Now all vc basic tests(e.g. di_vc, dr_vc, ii_vc tests) get data from mock service, and real data-provider tests are not included in this test.
I have modified some rust code, please help to take a closer look, I hope it will not affect worker logic.

Copy link

linear bot commented Feb 29, 2024

Copy link
Contributor

@Traf333 Traf333 left a comment

Choose a reason for hiding this comment

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

Overall looks good for me. Just one question came to my mind, what are the reasons to hardcode some paths within data providers like "v1/blocks/e4068e6a326243468f35dcdc0c43f686/children". Is the e406....f686 identifier permanent, or it might change in some cases?

Copy link
Collaborator

@BillyWooo BillyWooo left a comment

Choose a reason for hiding this comment

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

LGTM

@BillyWooo
Copy link
Collaborator

Overall looks good for me. Just one question came to my mind, what are the reasons to hardcode some paths within data providers like "v1/blocks/e4068e6a326243468f35dcdc0c43f686/children". Is the e406....f686 identifier permanent, or it might change in some cases?

I think it's simply moving around the hard code place.

@0xverin 0xverin merged commit 86857fa into dev Mar 5, 2024
31 of 34 checks passed
@0xverin 0xverin deleted the p-328-add-new-vc-tests branch March 5, 2024 03:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants