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

Refactor dynamic fields test ts sdk #7715

Merged
merged 3 commits into from
Jan 27, 2023
Merged

Conversation

siomari
Copy link
Contributor

@siomari siomari commented Jan 26, 2023

No description provided.

@siomari siomari requested a review from 666lcz January 26, 2023 13:18
@vercel
Copy link

vercel bot commented Jan 26, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated
explorer ✅ Ready (Inspect) Visit Preview 💬 Add your feedback Jan 27, 2023 at 10:02AM (UTC)
frenemies ✅ Ready (Inspect) Visit Preview 💬 Add your feedback Jan 27, 2023 at 10:02AM (UTC)
wallet-adapter ✅ Ready (Inspect) Visit Preview 💬 Add your feedback Jan 27, 2023 at 10:02AM (UTC)
1 Ignored Deployment
Name Status Preview Comments Updated
explorer-storybook ⬜️ Ignored (Inspect) Jan 27, 2023 at 10:02AM (UTC)

@vercel vercel bot temporarily deployed to Preview – frenemies January 26, 2023 13:18 Inactive
@vercel vercel bot temporarily deployed to Preview – wallet-adapter January 26, 2023 13:18 Inactive
sdk/typescript/test/e2e/dynamic-fields.test.ts Outdated Show resolved Hide resolved
expect(dynamic_fields.nextCursor).not.toEqual(null);
return await toolbox.provider
.getDynamicFields(parent_objectID, null, 1)
.then(async function (dynamic_fields) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
.then(async function (dynamic_fields) {
.then(async function (dynamicFields) {

The convention in TypeScript is to use camelCase for variable naming, could you fix it for all variables in this file?

@@ -106,5 +106,5 @@ export async function publishPackage(
const publishEvent = getEvents(publishTxn).filter(
(e: any) => 'publish' in e
)[0];
return publishEvent.publish.packageId.replace(/^0x(0+)/, '$1');
return publishEvent.publish.packageId.replace(/^0x(0+)/, '0x');
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for fixing this! I made a typo here, should be replace(/(^0x)(0+)/, '0x'); instead of replace(/^0x(0+)/, '0x');. Your change here is clearer

Comment on lines 60 to 66

return await toolbox.provider
.getDynamicFields(parent_objectID, dynamic_fields.nextCursor, null)
.then(function (dynamic_fields2) {
expect(dynamic_fields2.data.length).greaterThanOrEqual(0);
});
});
Copy link
Contributor

Choose a reason for hiding this comment

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

I actually don't think the refactoring of mixing await with callback makes sense. The bug seems to be in the backend rather than here

@666lcz 666lcz merged commit d0bca28 into main Jan 27, 2023
@666lcz 666lcz deleted the ms/dynamic_fields_test_ts_sdk branch January 27, 2023 20:49
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.

2 participants