Adding get method to Wrapper #1304

tomasbjerre · 2019-09-08T08:06:33Z

To have it fail with a clear error message when selector does not match an existing element.

lmiller1990 · 2019-11-19T22:44:08Z

Code looks fine, but is this additional API necessary? I don't fully understand the use case. If you cannot find something, you will surely get an error when you try to use it, anyway, or you will be making an assertion can get a decent output from the test runner.

tomasbjerre · 2019-11-20T07:24:30Z

you will surely get an error when you try to use it

Yes, and it will not be helpful.

I dont want a generic error like:

FAIL src/components/FCalendar/FCalendar.spec.ts (12.516s)
  ● expect next month button event to change currentMonth from 1 to 2

    [vue-test-utils]: find did not return .next-month-viewasd, cannot call trigger() on empty Wrapper

      31 |     expect(wrapper.vm.$data.currentYear).toBe(2019);
      32 |     expect(wrapper.vm.$data.currentMonth).toBe(1);
    > 33 |     wrapper.find('.next-month-viewasd').trigger('click');
         |                                         ^
      34 |     expect(wrapper.vm.$data.currentMonth).toBe(2);
      35 | });
      36 |

      at throwError (node_modules/@vue/test-utils/dist/vue-test-utils.js:1417:9)
      at ErrorWrapper.trigger (node_modules/@vue/test-utils/dist/vue-test-utils.js:2264:3)
      at Object.<anonymous> (src/components/FCalendar/FCalendar.spec.ts:33:41)

Instead I want something like:

FAIL src/components/FCalendar/FCalendar.spec.ts (12.516s)
  ● expect next month button event to change currentMonth from 1 to 2

    [vue-test-utils]: Unable to find .next-month-viewasd within: <div><span>whatever html was renderd and did not contain the element.....</span</div>

      31 |     expect(wrapper.vm.$data.currentYear).toBe(2019);
      32 |     expect(wrapper.vm.$data.currentMonth).toBe(1);
    > 33 |     wrapper.find('.next-month-viewasd').trigger('click');
         |                                         ^
      34 |     expect(wrapper.vm.$data.currentMonth).toBe(2);
      35 | });
      36 |

      at throwError (node_modules/@vue/test-utils/dist/vue-test-utils.js:1417:9)
      at ErrorWrapper.trigger (node_modules/@vue/test-utils/dist/vue-test-utils.js:2264:3)
      at Object.<anonymous> (src/components/FCalendar/FCalendar.spec.ts:33:41)

I often find myself doing console.log just to see what the dom looks like, because the error message does not help me.

lmiller1990 · 2019-11-20T10:16:16Z

I agree it's useful to have better debugging messages - but adding an entire API doesn't seem like the correct solution. Rather, can we improve find?

One idea could be for find to throw the error you described if it doesn't find an element. Then if you do wrapper.find(...).trigger, the initial find that found nothing would throw an error before trigger gets called. What do you think?

tomasbjerre · 2019-11-20T10:21:34Z

Changing find like that would break the API. Lots of people upgrading would get failing test cases because of that.

How would you assert that an element does, or does not, exist?

My point is that there is a difference between searching and getting. If you search with find, you may not find it, and that is totally ok.

lmiller1990 · 2019-11-20T12:44:04Z

Good point, I forgot the expect(wrapper.find('does-not-exist').exists()).toBe(false) type of assertion. I guess this could make sense.

I think the code is fine, will need to wait on a maintainer (@eddyerburgh , I guess?) to approve and merge.

Also, do you have a use case for getAll? It might be nice to match find and findAll APIs with get and getAll.

lmiller1990

Nice job using the existing functionality of find.

lmiller1990 · 2019-12-02T22:07:53Z

@tomasbjerre I'll be coming on board as a maintainer in the near future, and would like to get this one merged up I have the repo access to do so. I just realized since this does add a new API, we will need to add docs for it. Is this something you are able to do (if not, I can take a look at doing it).

tomasbjerre · 2019-12-03T06:23:23Z

Ok! I can make an attempt =)

tomasbjerre · 2019-12-03T17:59:53Z

I added some docs now.

lmiller1990

Just a small grammar improvement! Also, some tests seem to be failing.

docs/api/wrapper/get.md

tomasbjerre · 2019-12-04T13:00:04Z

The failing test seems to be toggling, looks like a timeout. I dont think that is introduced in this PR.

tomasbjerre

Changes made

lmiller1990 · 2019-12-05T13:49:37Z

LGTM, will merge when I get merge rights in the next week or two. Nice job!

lmiller1990 · 2019-12-16T11:48:02Z

I had another idea inspired by this repo in response to your earlier question, "How would you assert that an element does, or does not, exist?"

Could this make sense as a custom matcher? expect(wrapper.find('#el')).notToExist() reads and feels really nice to me, much more so than expect(wrapper.find('#el').exists()).toBe(false).

tomasbjerre · 2019-12-16T12:58:55Z

I often test a condition where I assume existens. And that is when the get method is useful

I don't want to spend my time and write code to test existens. I simply want to assume it exists and if it does not, i want a clear error. I'd like my test cases to be clear about other conditions and those conditions implicitly assumes the element actually exists. Like asserting the html() equals 5, the 5 ofcourse means that the element both exists and equals 5. In this case it is nice to have an error stating it was, perhaps, 4 not 5. And also, the implicit existens assertion, could fail with the element with selector X does not exist in html Y.

Looks like you are suggestion different ways of testing existens. And I argue that existens is not what I want to test but instead simply assume.
I also don't see how those assertions helps me figure out what the dom looked like. I want to avoid having to console.log the wrapper.html function.
I also don't see how anyone can browse the API and find such a method. If I type "wrapper." then my IDE will tell me there is a "get" method.

dobromir-hristov

What is the status on this? Generally this looks OK to me, hope ppl dont get confused when to use what.
You have added docs, so thats good.

docs/api/wrapper/get.md

afontcu · 2020-01-16T10:56:48Z

Should get also throw an error if more than 1 matching element is found?

lmiller1990 · 2020-01-16T13:20:35Z

@afontcu I think that it's fine not to throw an error for 1+ elements. If that is a use case, we should include a getAll function (perhaps getAll({ count: n })).

Although findAll would probably be fine for that.

lmiller1990 · 2020-01-16T13:33:24Z

Fixed merge conflicts, let's merge on CI passing. I wish GH had the feature GitLab has (auto merge on pass). We can dream!

tomasbjerre changed the title ~~Adding get method to Wrapper #1298~~ Adding get method to Wrapper Sep 8, 2019

Adding get method to Wrapper #1298

ea39a3f

To have it fail with a clear error message when selector does not match an existing element.

lmiller1990 approved these changes Nov 20, 2019

View reviewed changes

Documenting get method on wrapper #1298

7e3f227

lmiller1990 requested changes Dec 4, 2019

View reviewed changes

docs/api/wrapper/get.md Outdated Show resolved Hide resolved

Adjusting docs on get method on wrapper #1298

b41b951

tomasbjerre commented Dec 4, 2019

View reviewed changes

lmiller1990 approved these changes Dec 16, 2019

View reviewed changes

dobromir-hristov approved these changes Jan 15, 2020

View reviewed changes

afontcu reviewed Jan 16, 2020

View reviewed changes

docs/api/wrapper/get.md Show resolved Hide resolved

docs: update

726262c

lmiller1990 added 2 commits January 16, 2020 23:23

chore: merge in dev

a4ca2a1

chore: resolve conflict

a34e5b1

docs: linting

5ee369c

lmiller1990 merged commit dd7542d into vuejs:dev Jan 16, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding get method to Wrapper #1304

Adding get method to Wrapper #1304

tomasbjerre commented Sep 8, 2019 •

edited

Loading

lmiller1990 commented Nov 19, 2019

tomasbjerre commented Nov 20, 2019

lmiller1990 commented Nov 20, 2019

tomasbjerre commented Nov 20, 2019

lmiller1990 commented Nov 20, 2019 •

edited

Loading

lmiller1990 left a comment

lmiller1990 commented Dec 2, 2019

tomasbjerre commented Dec 3, 2019

tomasbjerre commented Dec 3, 2019

lmiller1990 left a comment

tomasbjerre commented Dec 4, 2019

tomasbjerre left a comment

lmiller1990 commented Dec 5, 2019

lmiller1990 commented Dec 16, 2019 •

edited

Loading

tomasbjerre commented Dec 16, 2019 •

edited

Loading

dobromir-hristov left a comment

afontcu commented Jan 16, 2020

lmiller1990 commented Jan 16, 2020

lmiller1990 commented Jan 16, 2020

Adding get method to Wrapper #1304

Adding get method to Wrapper #1304

Conversation

tomasbjerre commented Sep 8, 2019 • edited Loading

lmiller1990 commented Nov 19, 2019

tomasbjerre commented Nov 20, 2019

lmiller1990 commented Nov 20, 2019

tomasbjerre commented Nov 20, 2019

lmiller1990 commented Nov 20, 2019 • edited Loading

lmiller1990 left a comment

Choose a reason for hiding this comment

lmiller1990 commented Dec 2, 2019

tomasbjerre commented Dec 3, 2019

tomasbjerre commented Dec 3, 2019

lmiller1990 left a comment

Choose a reason for hiding this comment

tomasbjerre commented Dec 4, 2019

tomasbjerre left a comment

Choose a reason for hiding this comment

lmiller1990 commented Dec 5, 2019

lmiller1990 commented Dec 16, 2019 • edited Loading

tomasbjerre commented Dec 16, 2019 • edited Loading

dobromir-hristov left a comment

Choose a reason for hiding this comment

afontcu commented Jan 16, 2020

lmiller1990 commented Jan 16, 2020

lmiller1990 commented Jan 16, 2020

tomasbjerre commented Sep 8, 2019 •

edited

Loading

lmiller1990 commented Nov 20, 2019 •

edited

Loading

lmiller1990 commented Dec 16, 2019 •

edited

Loading

tomasbjerre commented Dec 16, 2019 •

edited

Loading