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

test(runtime-vapor): component props #99

Merged
merged 23 commits into from
Feb 4, 2024

Conversation

ubugeeei
Copy link
Member

@ubugeeei ubugeeei commented Jan 28, 2024

close #81


This is a new PR for #85!

I have implemented tests for the props runtime that I developed.
As mentioned in #74 (comment), it uses getCurrentInstance to access props in the code.

#99 (comment)

スクリーンショット 2024-01-31 0 19 03

Copy link

netlify bot commented Jan 28, 2024

Deploy Preview for vapor-repl ready!

Name Link
🔨 Latest commit b4063ba
🔍 Latest deploy log https://app.netlify.com/sites/vapor-repl/deploys/65bf8703e759ee0008459d9d
😎 Deploy Preview https://deploy-preview-99--vapor-repl.netlify.app/
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

github-actions bot commented Jan 28, 2024

Size Report

Bundles

File Size Gzip Brotli
compiler-dom.global.prod.js 79.7 kB 28 kB 24.6 kB
compiler-vapor.global.prod.js 44.6 kB 15 kB 13.6 kB
runtime-dom.global.prod.js 91.1 kB 34.7 kB 31.2 kB
runtime-vapor.global.prod.js 29.7 kB 11.6 kB 10.6 kB
vue-vapor.global.prod.js 71.9 kB 25.2 kB 22.8 kB
vue.global.prod.js 148 kB 53.9 kB 48.1 kB

Usages

Name Size Gzip Brotli
createApp 51.4 kB 20 kB 18.3 kB
createSSRApp 54.7 kB 21.3 kB 19.5 kB
defineCustomElement 53.7 kB 20.8 kB 19 kB
vapor 29.9 kB 11.6 kB 10.7 kB
overall 65.1 kB 25.1 kB 22.7 kB

@ubugeeei
Copy link
Member Author

Would it be better to test only the props without including the rendering process,
like in runtime-core/__test__/componentProps ( ? )

@ubugeeei
Copy link
Member Author

After all, I think I'll rewrite it a bit.

@ubugeeei ubugeeei removed the request for review from sxzz January 28, 2024 14:00
@ubugeeei ubugeeei marked this pull request as draft January 28, 2024 14:00
@sxzz
Copy link
Member

sxzz commented Jan 29, 2024

I think it would be better to have unit tests closer to the user side. (including rendering)

@ubugeeei
Copy link
Member Author

I was thinking of aligning it with the test cases of the traditional components to maintain compatibility, but what do you think?
Are both necessary (?)

@ubugeeei
Copy link
Member Author

Including render seems to inevitably give it a feel of an integration test.

Comment on lines 393 to 398
// NOTE: not supported
// optimized props update
// mixins
// validator
// warn
// caching
Copy link
Member Author

Choose a reason for hiding this comment

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

Is my understanding correct about this area?

Copy link
Member

Choose a reason for hiding this comment

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

for optimized props update: maybe it's unnecessary (I'm not sure for now). But Vapor can also pass the test. So let's add it first and then remove it if it's useless at the end of the development of Vapor.

Copy link
Member

Choose a reason for hiding this comment

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

validator should be implemented as well. Let's add it as test.todo

Copy link
Member

Choose a reason for hiding this comment

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

warn: What the reason is that you think it should be not supported? IMHO it should be also included.

Copy link
Member

@sxzz sxzz Jan 31, 2024

Choose a reason for hiding this comment

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

For the rest, let's drop them.

@ubugeeei
Copy link
Member Author

ubugeeei commented Jan 30, 2024

@sxzz

I have implemented the test based on the test cases of runtime-core/__test__/componentProps.spec.ts.

There are some points of uncertainty.


test result:

スクリーンショット 2024-01-31 0 19 03

@ubugeeei ubugeeei force-pushed the ubugeeei/test/runtime-component-props branch from fc30319 to 99ed6b3 Compare January 30, 2024 15:18
@ubugeeei ubugeeei marked this pull request as ready for review January 30, 2024 15:20
@sxzz
Copy link
Member

sxzz commented Jan 31, 2024

I was thinking of aligning it with the test cases of the traditional components to maintain compatibility, but what do you think?
Are both necessary (?)

We need two kinds of tests: test each single functions (something like test patchProp function) and you mentioned integration test (e2e): test API that the user will be using. (like createApp + ref...)

packages/runtime-vapor/__tests__/componentProps.spec.ts Outdated Show resolved Hide resolved
packages/runtime-vapor/__tests__/componentProps.spec.ts Outdated Show resolved Hide resolved
packages/runtime-vapor/__tests__/componentProps.spec.ts Outdated Show resolved Hide resolved
Comment on lines 393 to 398
// NOTE: not supported
// optimized props update
// mixins
// validator
// warn
// caching
Copy link
Member

Choose a reason for hiding this comment

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

for optimized props update: maybe it's unnecessary (I'm not sure for now). But Vapor can also pass the test. So let's add it first and then remove it if it's useless at the end of the development of Vapor.

Comment on lines 393 to 398
// NOTE: not supported
// optimized props update
// mixins
// validator
// warn
// caching
Copy link
Member

Choose a reason for hiding this comment

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

validator should be implemented as well. Let's add it as test.todo

Comment on lines 393 to 398
// NOTE: not supported
// optimized props update
// mixins
// validator
// warn
// caching
Copy link
Member

Choose a reason for hiding this comment

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

warn: What the reason is that you think it should be not supported? IMHO it should be also included.

Comment on lines 393 to 398
// NOTE: not supported
// optimized props update
// mixins
// validator
// warn
// caching
Copy link
Member

@sxzz sxzz Jan 31, 2024

Choose a reason for hiding this comment

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

For the rest, let's drop them.

@ubugeeei
Copy link
Member Author

We need two kinds of tests: test each single functions (something like test patchProp function) and you mentioned integration test (e2e): test API that the user will be using. (like createApp + ref...)

I created issue for props e2e test.

#105

@ubugeeei
Copy link
Member Author

@sxzz

Thank you for the review! I've made some revisions.
I've created a separate issue for the e2e tests, and I've put TODOs for warn and validate. (There are no additional implementations in this MR)

Regarding the caching of the default value, if I remove it, it might become unclear what the test is for, so for now, I'm leaving it commented out. (I wonder if it's okay to just delete it... It might be hard to tell if it was intentionally removed, so I feel it might be better to leave it for now and delete it later.)

@ubugeeei ubugeeei force-pushed the ubugeeei/test/runtime-component-props branch from fcf60d5 to c06f2f6 Compare January 31, 2024 14:49
@ubugeeei ubugeeei force-pushed the ubugeeei/test/runtime-component-props branch from c06f2f6 to c36257d Compare January 31, 2024 14:50
@ubugeeei ubugeeei requested a review from sxzz February 1, 2024 16:18
Copy link

netlify bot commented Feb 3, 2024

Deploy Preview for vapor-template-explorer ready!

Name Link
🔨 Latest commit b4063ba
🔍 Latest deploy log https://app.netlify.com/sites/vapor-template-explorer/deploys/65bf87036e48130008688ec4
😎 Deploy Preview https://deploy-preview-99--vapor-template-explorer.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@ubugeeei
Copy link
Member Author

ubugeeei commented Feb 3, 2024

Since pulling main, some tests are faild.
I'll fix it.

@ubugeeei
Copy link
Member Author

ubugeeei commented Feb 3, 2024

fixed.

@sxzz sxzz merged commit ea5f7ec into main Feb 4, 2024
8 of 9 checks passed
@sxzz sxzz deleted the ubugeeei/test/runtime-component-props branch February 4, 2024 12:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

runtime component props unit test
2 participants