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

chore: add test for nested v-model #2469

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

tharvik
Copy link

@tharvik tharvik commented Jul 9, 2024

reproduction as test for #2468, read there for context.

Copy link

netlify bot commented Jul 9, 2024

Deploy Preview for vue-test-utils-docs ready!

Name Link
🔨 Latest commit 2b989fb
🔍 Latest deploy log https://app.netlify.com/sites/vue-test-utils-docs/deploys/668d30efca0c450008874311
😎 Deploy Preview https://deploy-preview-2469--vue-test-utils-docs.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.

@nazarepiedady
Copy link
Contributor

@tharvik, I hope you're fine, could you edit your pull request to pass the CI?

@tharvik
Copy link
Author

tharvik commented Jul 17, 2024

could you edit your pull request to pass the CI?

this PR is meant to fail, it servers to show the issue underlined in #2468.

@nazarepiedady
Copy link
Contributor

@cexbrayat, @lmiller1990, is there already any solution for this problem?

@cexbrayat
Copy link
Member

@nazarepiedady No, I think Lachlan and I don't really have time to look into it, so you're welcome to try!

@nazarepiedady
Copy link
Contributor

@nazarepiedady No, I think Lachlan and I don't really have time to look into it, so you're welcome to try!

@cexbrayat, I will try it as soon as I finish something I am doing now.

@lmiller1990
Copy link
Member

lmiller1990 commented Jul 21, 2024

Good to have a reproduction for a known bug.

I think using findComponent is generally not a good idea - your test is now coupled to your component hierarchy, which means you are testing implementation details.

If someone wants to fix this, that would be great - this is the API we support, after all. I suspect you will need some hacks to do it, further complicating the code base. This indicates this kind of test and API is probably not the best idea.

Context for my opinion: I am working on a code base right now that has hundreds of these types of tests (findComopnent, setValue, etc). It is impossible to refactor or change anything, a lot of time and money was wasted both writing and maintaining this style of test.

@tharvik
Copy link
Author

tharvik commented Aug 2, 2024

any update on that? I came back today to bite me again and the workaround I was using is not working for deeper v-model.

@nazarepiedady
Copy link
Contributor

@lmiller1990, @cexbrayat, in your opinion, is a good idea to close this pull request and let a reminder on the documentation about why this is not a good idea?

@nazarepiedady
Copy link
Contributor

any update on that? I came back today to bite me again and the workaround I was using is not working for deeper v-model.

Did you understand the point presented by @lmiller1990?

@tharvik
Copy link
Author

tharvik commented Aug 5, 2024

any update on that? I came back today to bite me again and the workaround I was using is not working for deeper v-model.

Did you understand the point presented by @lmiller1990?

I did.
I didn't wanted to clutter the discussion with opinions but here goes.

I think using findComponent is generally not a good idea - your test is now coupled to your component hierarchy, which means you are testing implementation details.

yeah, I agree, and totally get that it makes it harder to change the implementation. but as I want to test a component wrapping a form made of many custom fields. I can either set the value of every single field, or just set the resulting form model which is way more readable.

FWIW: I've got a bunch of layers of v-model, each making a bit of the needed parsing (it's transforming inputed values as it updates along the hierarchy), smth around the line of DatasetInput > ImageDatasetInput > FileInput > input. the model type at each layers is different with added context at every step.
I got to where I'm now also because I didn't find a way to setValue on a <input type=file /> thus disallowing me testing the stack without setValue on the layer below.

If someone wants to fix this, that would be great - this is the API we support, after all. I suspect you will need some hacks to do it, further complicating the code base. This indicates this kind of test and API is probably not the best idea.

it all depends on the needs of projects. if setValue was to be removed, I'll probably workaround it by trying to set modelValue myself. I think that the API is helpful as to make the tests more readable.

@lmiller1990, @cexbrayat, in your opinion, is a good idea to close this pull request and let a reminder on the documentation about why this is not a good idea?

well, I find it pretty undelicate to tell devs that's their code is badly though of. they are the most knowledgable on their codebase and what are its specific needs.
also, closing a PR triggering a bug is akin to putting it under the rug: it will come back.

@nazarepiedady
Copy link
Contributor

@tharvik, is there any specification or documentation that says that this interface should work the way you're pointing?

@nazarepiedady
Copy link
Contributor

@cexbrayat, @lmiller1990 is @tharvik, right?

Is this a bug?

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.

4 participants