search

vuejs / vue-test-utils

Component Test Utils for Vue 2
https://vue-test-utils.vuejs.org
MIT License
3.57k stars 670 forks source link

Rethink find; simplify, or split find into find and findComponent #1498

Closed lmiller1990 closed 4 years ago

lmiller1990 commented 4 years ago

Following up this discussion here: https://github.com/vuejs/vue-test-utils/issues/1490. Read that for context!

I think we should consider splitting find into two methods.

Usage

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.

unclejustin commented 4 years ago

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: wrapper.findComponent(Component, { ref: 'componentOne' }). But it always expects to look for an actual Component.

Stoom commented 4 years ago

Would using ref require all components to have a defined ref attribute? If so I'd be for component name or component.

lmiller1990 commented 4 years ago

@Stoom I don't know if I understand your question. find({ ref: 'blah' }) looks for a component with a ref defined by the user.

@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 find("#foo") and find(Component)? By use case, I mean an actual test you can't write without finding by ref. One of my personal goals for any library/app I build is to provide the minimal API needed to solve all the use cases.

If you need to use ref, you are most likely testing implementation details, which is a bad practice. We should not encourage bad practices. But there might be some legitimate use cases I don't know about.

Stoom commented 4 years ago

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 { name: 'foobar' } I think. This comes from working on the stubbing.

lmiller1990 commented 4 years ago

name is another option. The problem I see with name over component is finding third party components that may not have name.

Now I think about it, I wonder if we should consider deprecating finding components entirely (ref/name/instance) and only support querySelector syntax.

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:

JessicaSachs commented 4 years ago

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 <v-sheet> instead of a div, my tests will break because I'm suddenly looking for a component instead of a dom element.

IMHO, the best solution would be to support querySelector syntax and ask users to use data-testid on their components.

This will normalize finding across DOM nodes and components. Additionally, data-testid is resilient as a selector strategy to many kinds of refactors.

lmiller1990 commented 4 years ago

I agree with @JessicaSachs - I exclusively use find("[data-test='blah']") for this reason. The way I test Vuetify is like this:

<template>
  <v-btn data-test="go">Go</v-btn>
</template>
wrapper.find('[data-test="go]').find('button').trigger('click')
souldzin commented 4 years ago

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.

IMHO, as a client of .find I don't really care how it's searching. I just want it to return me a wrapper based on the query I'm giving it.

Okay @souldzin, so why do you like keeping the single .find method?

const FOO_QUERY = '.foo';

it('...', () => {
  wrapper.find(FOO_QUERY)
  // ...
});

// etc.

Currently, if I look at my code and I realize that we actually have a ref we can use, I can easily update my tests with a single line and I'm done:

- const FOO_QUERY = '.foo';
+ const FOO_QUERY = { ref: 'foo' };

If looking up by component required me to call a whole other method, then I'd have to change every reference to FOO_SELECTOR... Also, it'd make it really hard to write a helper like:

const findFooChild = (sel) => wrapper.find(Foo).find(sel);

It's worth considering the robustness we'd be losing.

But!

If vue-test-utils blows up with:

wrapper.find('.dom-element').find(FooComponent)

Then I'd say vue-test-utils has a :bug:

I much prefer the Python motto "There should be one-- and preferably only one --obvious way to do it".

I'd argue that this supports having a single .find method which accepts various parameters rather than having two methods .find and .findComponent :thinking:

lmiller1990 commented 4 years ago

Great points above @souldzin. Here is my thoughts:

IMHO, as a client of .find I don't really care how it's searching. I just want it to return me a wrapper based on the query I'm giving it.

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.

Developer expectations. I imagine that part of vue-test-utils API is inspired by enzyme. Keeping this parity would be really nice.

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 testing-library from what I can see, ditching things like find(Comp) and shallow. testing-library is recommended in the official React docs, and included in create-react-app by default. We have no need to follow what Enzyme does, although I have a lot of respect for the creators of Enzyme, they are very smart.

If looking up by component required me to call a whole other method, then I'd have to change every reference to FOO_SELECTOR...

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 data-testid selector.

I'd argue that this supports having a single .find method which accepts various parameters rather than having two methods .find and .findComponent 🤔

I do agree two methods is more complex than one.

JessicaSachs commented 4 years ago

My vote is to reduce find's support to querySelectors only. I also think @souldzin is right that there is an existing bug.

I also only want one method.. let's stick to find.

Why do we want to support querySelectors instead of component references?

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.

souldzin commented 4 years ago

Thanks for the responsiveness @lmiller1990! Also thanks for creating the discussion :smile:

Strong disagree here. While VTU may seem Enzyme inspired, it's really a relic of the time it was created

Thanks for this info! I may have inadvertently revealed when the last time I wrote React test code was :sweat_smile:

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?

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.

My vote is to reduce find's support to querySelectors only.

@JessicaSachs This is really interesting :thinking:

At my organization, we do a lot of shallowMount unit testing (for better or for worse) and IMO it's really nice to expect(wrapper.find(FooComponent).props()).toEqual(..) so that I know this is where my current unit test can stop and the unit tests for FooComponent can begin.

unclejustin commented 4 years ago

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. data-testid which is arguably the same amount of work as ref="whatever". I think as long as find works consistently and obviously I'm going to be happy 😅 Although I'm not super pumped about all the broken tests my organization will have to fix 😬

JessicaSachs commented 4 years ago

@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.

export const FooSelector = '[data-testid=my-semantic-name]'
...

@unclejustin thank you for the discussion.

lmiller1990 commented 4 years ago

Great contribution so far everyone! This is why OSS is great - anyone can contribute and have an impact on a project.

@unclejustin

Although I'm not super pumped about all the broken tests my organization will have to fix 😬

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 expect(wrapper.find({ ref: 'foo' }).vm.msg).toBe('Welcome, Foobar') instead of expect(wrapper.html()).toContain('Welcome, Foobar'). One tests implementation, the other tests the public API. I wonder which is better for the user, an in memory greeting or one they can actually see 🤔

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

expect(wrapper.find(FooComponent).props()).toEqual(..)

To me this indicates that FooComponent is going to do something with the props - and eventually render content based on those props. Instead of two sets of unit tests (one for Parent, one for FooComponent, get can get a better bang for your buck by just testing them both in one go?

I'm pretty curious how other people view this... shallowMount and .props() seem to be widely used - I believe this is not ideal, since you don't actually verify what the component should be doing in production - that is, rendering some content based on the props or user interactions via trigger.

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).

afontcu commented 4 years ago

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:

IMHO, the best solution would be to support querySelector syntax and ask users to use data-testid on their components.

Now, as a disclaimer I'd say that I never use shallowMount and/or .props(), mostly because it's opposed to (1), so I could fail to see some legitimate use cases. I'd rather treat components as black boxes, and assert their public API – that is, the rendered DOM + the emitted events + additional side effects such as console.logs, HTTP requests, whatever.

xanf commented 4 years ago

I still prefer having one way to do things, so having one method is greater than having two

IMHO, the best solution would be to support querySelector syntax and ask users to use data-testid on their components.

There is at least one point I would like to raise about removing "other styles" of searching - sometimes I'm putting ref to renderless components and building my assertions against them.

Why I'm doing that? For big projects I find approach with mount and asserting HTML extremely fragile, resulting in tests, being broken by unrelated changes. For me usage of shallowMount and .props() is important to capture component logic about passing data between various components (yes, I'm treating my test as unit test, not component or integration test)

Instead of two sets of unit tests (one for Parent, one for FooComponent, get can get a better bang for your buck by just testing them both in one go?

This really breaks the idea of tests, showing "what is broken" - now I will get red test if either Parent or FooComponent is broken. In huge codebases this does not help at all

I'd rather treat components as black boxes, and assert their public API – that is, the rendered DOM

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

lmiller1990 commented 4 years ago

Hi!

There is at least one point I would like to raise about removing "other styles" of searching - sometimes I'm putting ref to renderless components and building my assertions against them.

Can you give an example? I'm not sure I understand - I really want to understand the different use cases people have.

resulting in tests, being broken by unrelated changes.

I definitely don't understand this. If you made a change and the test broke, then your change broke the test, right?

unclejustin commented 4 years ago

I'm confused about some of the shallowMount talk here. @JessicaSachs @afontcu it sounds like you are saying to never use shallowMount.

If I have a component with dozens of child components that are nested multiple levels deep isn't shallowMount the ideal way to compartmentalize those tests?

lmiller1990 commented 4 years ago

you are saying to never use shallowMount

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 shallowMount if it gives you confidence in the code you are shipping. I don't use it because it doesn't give me confidence in my code, since it's much less production-like than mount.

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 find for now :)

lmiller1990 commented 4 years ago

Here is another example of why I think we should drop find(Comp): https://github.com/vuejs/vue-test-utils/issues/1351

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 findComponent to return a VueWrapper.

querySelector (and even ref and name) are better since they are not ambiguous. Out of find(Comp/ref/name), I think ref is the least ambiguous.

Stoom commented 4 years ago

Another pro for the data-testid is the independence between mounting and shallow mounting. I've noted that personally and it's a pretty nice step forward. If you're currently using shallow mount and want to transition to a full mount then the test id is a good way for things not to break.

JessicaSachs commented 4 years ago

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 shallowMount. Feel free to find me in the VueLand Discord to discuss further.

My views on find are still the same

IMHO, the best solution would be to support querySelector syntax and ask users to use data-testid on their components.

So @lmiller1990 lemme know what you think about the below...

Proposal

Promote selection of elements via DOM output instead of component references or Vue-specific syntax.

✅ find("[data-testid=foo").find("[data-testid=bar]") This is the preferred method of selecting elements. It's the least brittle. If you would like to have a syntax for find(foo).find(bar) then make string constants like so:

// Ideally this object would live in a constants file
// or you could add it to global in your test setup
const { foo, bar } = {
  foo: '[data-testid=foo]',
  bar: '[data-testid=bar]',
  ...
}

find(foo) // Wrapper
findAll(bar) // WrapperArray

🤔 find({ ref: 'foo' }) // <div ref="foo"/> and find({ name: 'foo' }) // <foo/> I don't think these add more value than data-testid. Would vote to remove to simplify the API

❌ find(Foo).find(Bar) Component references are ambiguous and may break your tests during refactors. I would like to discourage use of this pattern and preferably remove the signature altogether.

lmiller1990 commented 4 years ago

Great summary @JessicaSachs . ref and name may make sense. I don't fully understand @xanf 's use-case (I'm dumb). I will need a concrete example. I would vote to remove them if they simplify the API - only if their removal doesn't make it impossible to test something (which is why I'd like to see a example with a .vue file and a .spec.js file, where ref is necessary to test the component).

Based on my observations, find({ ref: 'foo' }) will not be nearly as useful for Vue 3, since the new design of Vue 3 does not expose vm in the same way Vue 2 does. This means finding a component instance is not very useful anymore. Unless Vue 3 changes (which it might, still in alpha) I don't think finding an instance is useful anymore, since you cannot see props or data from the outside anyway. This is something not a lot of people have realized yet.

I agree data-testid is robust and resilient. Robust is good. Resilient is good. I have used data-testid for years in many projects, from selenium to cypress to VTU, with great success.

I don't lke typing find("[data-testid]"). It's difficult to type and hard to read. I would be in favor of including a helper, or even have something built in the when you do find('blah') it will do find('[data-testid="${blah}"]') to save some keystrokes. Of course this would be in addtion to the querySelector syntax. I believe cypress, testing-framework and others do this.

JessicaSachs commented 4 years ago

Convenience methods to encourage use for data-testid would be very welcome. Let’s break that out into another issue.

JessicaSachs commented 4 years ago

@lmiller1990 I would suggest reading this from Angular on Component Class testing to get some deeper understanding on @xanf’s unit testing world view

lmiller1990 commented 4 years ago

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 find(Comp), though.

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 ref, where querySelector would not suffice.

xanf commented 4 years ago

@lmiller1990 Sorry for delay in response, I will try to build a separate example within 24hrs :)

cexbrayat commented 4 years ago

My 2cts coming from the Angular world (I'm a long time Angular contributor): I really like that find accepts both a DOM selector or a component. It is way less verbose than Angular where you have two separate (and longer) methods. Keeping find(Foo) or at least findComponent(Foo) would be great (and would greatly simplify migrations).

dobromir-hristov commented 4 years ago

We are leaning towards find and findComponent. It would allow for advanced usage, while keeping things easier to reason with.

I am almost ready with the implementation and it's looking promising :)

souldzin commented 4 years ago

Here is another example of why I think we should drop find(Comp): #1351

You expect a VueWrapper - but you get a DOMWrapper.

@lmiller1990 Looking at the docs, I'd expect to get a Wrapper. From the eyes of a client, this is an interface I should be able to consistently use despite it's underlying implementation VueWrapper or DOMWrapper 🤔

❌ find(Foo).find(Bar)

Component references are ambiguous and may break your tests during refactors. I would like to discourage use of this pattern and preferably remove the signature altogether.

I might be missing something, but are we considering removing querying by component altogether now?

IMHO, finding by component is really helpful for shallowMount tests. This helps enforce that I don't need to test the details of Foo in this spec.

Yeah @souldzin, but mount tests actually cover a lot more ground, and we'd like to encourage users towards that direction.

I really like Martin Fowler's analysis of the tradeoffs between Mockists and Classicist.

mount and shallowMount are different levels of testing, each with their own pro's and con's. TDD'ing from the perspective of a user with mount is awesome (and my personal preference), but it isn't free. There's some application states which are difficult to get to in full integration (e.g. step 3 of a highly interactive single page form). Even if I build a test helper to get there, my tests suite is going to be doing a lot of redundant work to trigger this state in an integrated environment. This is where the profitability of shallowMount unit tests can come in.

IMO, when it comes to mount vs. shallowMount, these are both valuable nets to have. And if we're keeping shallowMount, I'd strongly suggest keeping find(FooComponent) because this helps enforce the completeness of a mocked unit test.

souldzin commented 4 years ago

✅ find("[data-testid=foo").find("[data-testid=bar]") This is the preferred method of selecting elements. It's the least brittle. If you would like to have a syntax for find(foo).find(bar) then make string constants like so:

devils advocate: This approach favors full mount. In shallowMount tests, replacing find(Component) with find("[data-testid=foo]") causes me to lose my assertion that the "mock/stubbed" component was actually "called". At that point, why use @vue/test-utils at all? Why not just .$mount to jsdom and use the good ol' DOM api? 🤔

lmiller1990 commented 4 years ago

Again, good feedback!

I might be missing something, but are we considering removing querying by component altogether now?

It looks like this will become findComponent, since it is fundamentally different from finding a DOM node. The separation will make things more clear and easier to understand. The migration path is very easy, we can do a codemod.

So if you are a shallowMount(Foo).find(Bar) person, you'll have no problem migrating (except your tests are not as production-like as they could and should be, but that's the your problem, not VTU's problem).

full integration (e.g. step 3 of a highly interactive single page form). profitability of shallowMount unit tests can come in

Each to their own, but I think these are conflicting ideas. You want an integration test for a highly complex form... so you shallowMount it. You don't have an integration test anymore, you have a form with no inputs since you stubbed them out, which is not an integration test.

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 find -> findComponent and a simple codemod to migrate, I'd like to close this. I'll leave it open for a few days to see if anyone else would like to voice an opinion.

xanf commented 4 years ago

Sorry for delay with providing example (deliverables, sigh :)), jumping on that right now. Meanwhile I have important question

except your tests are not as production-like as they could and should be, but that's the your problem, not VTU's problem.

@lmiller1990 your wording is very strong here, so I must ask - is this an official VTU team position or your private one?

lmiller1990 commented 4 years ago

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 mount is more production like than shallowMount. Components are just abstractions that make it easier to reason about code and relationships between modules.

Opinions aside, vue-test-utils-next allows you to find a component by name, ref and component. This should allow you to write whatever kind of test case you would have previously written. I think this should make migrating tests from Vue 2 to Vue 3 easy.

The only change is we are looking to name the API to find a component findComponent in VTU next (not this repo, the Vue 3 support). find will be dedicated to finding DOM nodes. It is a work in progress, but seems to be working fine and the PR is here.

For upgrading, find(Foo) etc just becomes find(FooComponent). It is an easy code mod. Alternatively, it would be trivial to provide a bridge or some kind of adapter to make find work on components for very large codebases like GitLab (or you could do it in userland) to make migrating easier.

What do you think?

BTW we started using Gitlab at works it's awesome, you and the team rock.

souldzin commented 4 years ago

The only change is we are looking to name the API to find a component findComponent in VTU next (not this repo, the Vue 3 support).

While this isn't ideal, I'm not familiar with the details of @vue/test-utils to provide a good alternative. Thanks for looking into it and creating this dicsussion @lmiller1990!

For upgrading, find(Foo) etc just becomes find(FooComponent). It is an easy code mod.

Heads up! There's some non-trivial cases that would not be easy to migrate.

// test helper
const findAtRow = (i, selector) => {
  return wrapper.find('table tr').at(i).find(selector);
};

A codemod will not be able to easily pick up whether selector is a component, or not, or both 😞... Please keep in mind that splitting up a function like this causes things which wrap VTU to be split up or duplicate that logic. That's not the end of the world, but it's certainly a cost factor to be considered in this decision.

suggestion: If we're pretty set on splitting up find and findComponent, could we make sure that find(Component) blows up with a clear message so that we don't accidentally create hard to debug situations, or worse... false positive tests.

JessicaSachs commented 4 years ago

I still agree with @souldzin that splitting find and findComponent is a bad idea. It couples the source implementation with the test case worse than shallowMount does alone.

Scenarios

Scenario 1 - VTU v1

  1. User makes a component <v-sheet>Foo</v-sheet> and tests using find
  2. User upgrades VTU
  3. The tests break
  4. All usages of find where a component is selected must change find => findComponent

Scenario 2 - VTU v2

  1. User makes a component <div>Foo</div> and tests using find
  2. User refactors their code to use a wrapper component: <v-sheet>Foo</v-sheet>
  3. The tests break
  4. Any dependent tests must change from find => findComponent

Summary

  1. Scenario 1, in theory, would be solved by codemod -- but this is not proven out.
  2. Scenario 2 would always be a tax on the user.

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?

@afontcu @dobromir-hristov

dobromir-hristov commented 4 years ago

So should find be able to return Vue instances or not @JessicaSachs ?

The user can use find to query dom nodes as much as they desire. If they change a DOM Node to a Vue wrapped component, their tests would still work, because they dont need a Vue wrapper to do those DOM assertions.

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 find was going to be DOM selector only.

lmiller1990 commented 4 years ago

@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 #, ., or a HTML element, they are querying a DOM element. Otherwise it's component. Am I missing something? There are some edge cases, like const sel = '#sel'; find(sel). This not that common.

It couples the source implementation

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 find(Foo) API sucks. It lets encourages you to write bad tests - components are implementation details, just an abstraction on the DOM. If you write your tests like this, you are going to have this problem anyway.

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

Finding a component instance is deeply embedded in many companies test suits

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.

JessicaSachs commented 4 years ago

Yeah this is really constructive and much easier to follow than Discord, lol

~So should find be able to return Vue instances or not @JessicaSachs ?~

~@dobromir-hristov yes I think so. Thanks for implementing this in vue-next.~

~@lmiller1990 ~

~This is why the find(Foo) API sucks. It lets encourages you to write bad tests - components are implementation details, just an abstraction on the DOM. If you write your tests like this, you are going to have this problem anyway.~

~Ignoring any breaking migration, here is a scenario where the user is writing a great test, but our new find/findComponent API bites them:~

~An ambiguous selector find('[data-testid=whatever]'):~ ~ <div data-testid="whatever"/>~ ~ Refactor this to use a component: <v-sheet data-testid="whatever"/>~ ~ Existing tests fail because v-sheet is a component~ ~ You have to refactor your test code to use findComponent when the result of that selector now returns a component?~

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: Users shouldn't need to change their tests if they are functionally the same. VueWrapper and DOMWrapper should implement the same basic "visible", "text" methods and the methods for finding nodes should be agnostic to if those things are raw DOM nodes or Vue nodes.

lmiller1990 commented 4 years ago

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.