-
Notifications
You must be signed in to change notification settings - Fork 668
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
Rethink find; simplify, or split find into find and findComponent #1498
Comments
I really like the idea of having one good way to select something as long as you have some way to pass in optional arguments. Like: |
Would using |
@Stoom I don't know if I understand your question. @unclejustin Having one way to select things with optional arguments isn't one way... it's one way + the amount of arguments you provide. Is there any use case that can't be solved by If you need to use |
Ok, that's what I was thinking. If we do find my component then you could also do find my name by doing something like |
Now I think about it, I wonder if we should consider deprecating finding components entirely (ref/name/instance) and only support As convenient as it can be to find by component, I personally prefer libraries that are more simple with an API that consistently works like you expect for all use cases. Sometimes less is more. Here are some examples that do not work in beta that come up a lot:
|
I'm not in favor of the proposal as it violates our principle of not letting implementation details into our tests. If I refactor my component to use a IMHO, the best solution would be to support This will normalize finding across DOM nodes and components. Additionally, |
I agree with @JessicaSachs - I exclusively use <template>
<v-btn data-test="go">Go</v-btn>
</template> wrapper.find('[data-test="go]').find('button').trigger('click') |
IMHO, as a client of Okay @souldzin, so why do you like keeping the single
|
Great points above @souldzin. Here is my thoughts:
I agree with this. The user should not need to think about how things work. They have better things to do, like build apps and deliver value.
Strong disagree here. While VTU may seem Enzyme inspired, it's really a relic of the time it was created - Enzyme was basically the only testing lib for components out there. Since then, the React world moved on to
If your tests broke when you refactored your code, this is a big red flag - yours tests are fragile. If you didn't change the public API and behavior, why are the tests failing? If you need to select a specific element, the solution is to use a dedicated
I do agree two methods is more complex than one. |
My vote is to reduce find's support to I also only want one method.. let's stick to
Because the DOM is the universal language for querying the rendered output. Maintaining another abstraction gets particularly hairy when we lose the component reference in shallowMount or with stubs. |
Thanks for the responsiveness @lmiller1990! Also thanks for creating the discussion 😄
Thanks for this info! I may have inadvertently revealed when the last time I wrote React test code was 😅
Right. Sorry if this example was misleading. The idea is about maintaining existing test code. It's much easier for me to write domain-specific test helpers when I don't have to be worried about this function split.
@JessicaSachs This is really interesting 🤔 At my organization, we do a lot of |
Also, thanks for the discussion! The more I try and come up with some rando scenario for using refs with a Component selector the more I can see a way around it i.e. |
@souldzin I'd recommend maintaining a file of selectors for convenience. Also, look into Page Object Model if you want to really get into DSL's for tests. The angular docs seem to have deep love for the Page Object Model.
@unclejustin thank you for the discussion. |
Great contribution so far everyone! This is why OSS is great - anyone can contribute and have an impact on a project.
Don't worry... I joined an org with a large test suite that does similar things. We refactor to remove tech debt and tests fail, despite changing no behavior. This is because they do things like Things are slowly getting better, though! People cannot be blamed - it's mostly our fault of our docs and API, for providing tools to do the wrong thing, and not explaining what to do (and what not to do). @souldzin I find these tests intriguing
To me this indicates that I'm pretty curious how other people view this... We should definitely do a review of the goals of VTU and what kind of things people want to test (not what kind of tests they want to write). |
This is a great discussion so far – thanks everyone for the contributions! My take is that VTU should (1) stay away from suggesting to rely on implementation details, (2) provide a simple API – which means a single way of doing things, if possible, and (3) teach and explain this approach, so that people does not feel lost. This means not trying to follow paths such as Enzyme's (it would break (1) and (2)). Thus, as @JessicaSachs said:
Now, as a disclaimer I'd say that I never use |
I still prefer having one way to do things, so having one method is greater than having two
There is at least one point I would like to raise about removing "other styles" of searching - sometimes I'm putting Why I'm doing that? For big projects I find approach with
This really breaks the idea of tests, showing "what is broken" - now I will get red test if either
Strong support for treating component as black box, but from my point of view the result of the component is virtual dom, and I'm building my assertions around it. I don't want to know that some deep-deep child require additional provides/injects to work, etc. for example |
Hi!
Can you give an example? I'm not sure I understand - I really want to understand the different use cases people have.
I definitely don't understand this. If you made a change and the test broke, then your change broke the test, right? |
I'm confused about some of the If I have a component with dozens of child components that are nested multiple levels deep isn't |
That is what they are saying. The argument is you should render the entire DOM and test it you like users will use it. Read this for some more context. Use We will continue to support both mounting methods. If you want to talk about more design patterns for testing, we can do so in the Vueland discord, feel free to ping me. Let's try and keep this discussion limited to |
Here is another example of why I think we should drop You expect a VueWrapper - but you get a DOMWrapper. This makes sense - functional components don't have instances, they are basically just stateless DOM elements with props - but it's still confusing, you would expect
|
Another pro for the |
I hear you @xanf. You're trying to use your components as containers for your business logic, and you're asserting on their public API (methods). Totally reasonable. That's the exact reason we're keeping My views on
So @lmiller1990 lemme know what you think about the below... ProposalPromote selection of elements via DOM output instead of component references or Vue-specific syntax.
✅
🤔 ❌ |
Great summary @JessicaSachs . Based on my observations, I agree I don't lke typing |
Convenience methods to encourage use for |
@lmiller1990 I would suggest reading this from Angular on Component Class testing to get some deeper understanding on @xanf’s unit testing world view |
Sure, I get testing components as a regular class (not considering DOM or rendering bahavior specifically, but the business logic in methods etc). I am not sure how this relates to What I don't understand is this part of the comment: "I'm putting ref to renderless components and building my assertions against them.". To me, this sounds like a description of a specific use case for finding by |
@lmiller1990 Sorry for delay in response, I will try to build a separate example within 24hrs :) |
My 2cts coming from the Angular world (I'm a long time Angular contributor): I really like that |
We are leaning towards I am almost ready with the implementation and it's looking promising :) |
@lmiller1990 Looking at the docs, I'd expect to get a
I might be missing something, but are we considering removing querying by component altogether now? IMHO, finding by component is really helpful for Yeah @souldzin, but
|
devils advocate: This approach favors full |
Again, good feedback!
It looks like this will become So if you are a
Each to their own, but I think these are conflicting ideas. You want an integration test for a highly complex form... so you For large, complex integration tests, there are other libraries that are better suited to this problem, imo, namely testing-library, which uses VTU internally. They aren't competing libs - just different tools. Unless someone is extremely unhappy with |
Sorry for delay with providing example (deliverables, sigh :)), jumping on that right now. Meanwhile I have important question
@lmiller1990 your wording is very strong here, so I must ask - is this an official VTU team position or your private one? |
I cannot speak on behalf of anyone else without their permission. It's my opinion - I know some others share it, but I'll let them speak for themselves. Browsers render DOM nodes, not components, and for this reason Opinions aside, The only change is we are looking to name the API to find a component For upgrading, What do you think? BTW we started using Gitlab at works it's awesome, you and the team rock. |
While this isn't ideal, I'm not familiar with the details of
Heads up! There's some non-trivial cases that would not be easy to migrate.
A codemod will not be able to easily pick up whether suggestion: If we're pretty set on splitting up |
I still agree with @souldzin that splitting ScenariosScenario 1 - VTU v1
Scenario 2 - VTU v2
Summary
In both scenarios, we are doing the thing we don't agree with: causing implementation details to leak and fall on the shoulders of our users. @lmiller1990 Scenario 2 is still gonna bite users. It's worse than the implementation leak of shallowMount because it affects all users, regardless of mounting strategy and component hierarchy. Scenario 2 is enough for me to want to avoid it. Am I missing something? Do we have enough info to come to an agreement as a team? |
So should The user can use Finding a component instance is deeply embedded in many companies test suits. The PR I made was to at least give them a chance to do that again, seeing as |
@JessicaSachs thanks for voicing your opinion. Disagreement is good - it lets us explore the issue more fully. We can easily tell if a selector is a component or not - if it starts with
I don't understand what you mean here - may need some clarification. Maybe I wasn't clear here - this is only going to change from beta (aka v1) to v2 - not in this code-base. Users will only need to upgrade their code once - again, code-mod should be mostly doable. It's a major version, we can have some breaking changes if they make sense and provide a migration path. In scenario 2, the reasons the tests are breaking is because they are testing implementation details, right? finding a specific component is an implementation detail. Refactoring should not break your tests. This is why the The reason I like this change is it gives some separation between the DOM testing and component testing utilities. Bundling them in the same function make it seem like they do the same thing, when really they do something completely different, and operate on completely different levels of abstraction. Just saw @dobromir-hristov's reply
Yep, so we need to support it. Thanks for implementing that in VTU next. I think providing some separation will help new users understand the difference between finding a component (an abstraction) and a DOM (what you actually render), and hopefully encourage them to do the latter. |
Yeah this is really constructive and much easier to follow than Discord, lol
~@lmiller1990 ~
I grossly misunderstood the proposed API. My misunderstanding was that we were discussing what was being found, not the input type of what was being passed into find. I still stand by: |
Hi! Maybe I did a bad job explaining. Yep, the found thing (VueWrapper or DOMWrapper) will implement the same API. We enforce this via this interface. To clarify the only proposed change is the name of the function. Before
** After**
That's it. @dobromir-hristov will post an RFC soon that puts this into more format terms and we can use this discussion as a reference. I will close this for now, and when the RFC is opened I will update this with a link to said RFC. I think this discussion was useful and I appreciate all the contributors. |
Following up this discussion here: #1490. Read that for context!
I think we should consider splitting
find
into two methods.find
. This will only work for DOM nodes. It will use thequerySelector
syntax.findComponent
. This can find a Vue component. By ref, name, etc.Usage
find("#foo")
findComponent(Bar)
findComponent(Bar).findComponent(Foo)
findComponent(Bar).find("#foo")
find("#foo").findComponent(Bar)
The reason I think we should split these is they are really different. One is searching the DOM for nodes; the other is searching for vnodes in Vue's virtual dom. I think this would remove a lot of caveats and simplify things.
I think not allowing them to mix will make tests more simple. Simple is good.
findComponent
tells the reader you are interested in the component;find
shows you are interested in the DOM and what is rendered.We can either make this change now (with a deprecation warning) to be removed in v1 (hopefully soon). Alternatively, we keep this current behavior, and for v2 (which will be Vue 3 support) we change to this new API.
As an aside, I would be in favor of making
findComponent
more simple - currently you can find by name/ref/component... can we just pick one? Rather than go with the classic Perl motto "There is more than one way to do things" I much prefer the Python motto "There should be one-- and preferably only one --obvious way to do it". Anyway, let's focus on the splitting of the APIs for now.The text was updated successfully, but these errors were encountered: