Vue-carousel: Chore: Increase Code Coverage!

[quinnlangille]

The last few updates have had dropped coverage significantly - I'd like to re-add coveralls checks, but before we do that, it would be nice to have our code coverage/quality be above 80%. This is a great opportunity for someone to learn/practice writing unit tests. If anyone has any questions, feel free to comment here!

I'll leave this issue up for a few weeks, but if we have no biters I'll add them before we move to v7.0 🏁

[RyanLiu0235]

I'd like to do this.🙋But it seems that I have claimed too many tasks. I'll do it one by one.

[quinnlangille]

Yes! Getting the coverage increased is going to be really important as we move to v1.0.0 - it would be a H U G E help if you could make some contributions here 😁. No worries on the number of PR's, but I agree that we should resolve #165 first!

[RyanLiu0235]

Hey @quinnlangille check this out. What about migrating tests to vue-test-utils? We can still use Jest but things will be a lot easier.

[quinnlangille]

@stop2stare we're actually doing that migration at ssense right now, and it would be great to have it here as well. If you feel like you're up for taking on the migration, it would be a huge boost for our codebase

[RyanLiu0235]

@quinnlangille great, what's your progress about migration? How can I participate in your current work?

[quinnlangille]

We're not migrating any open source yet, just our internal vue repos so feel free to refactor how you see best!

[RyanLiu0235]

@quinnlangille oh I see, that's ok.

[josephting]

Before working on increasing the code coverage, I'm looking to incorporate Vue Test Utils which means major test script refactor.

With that said, current test methodology is using mount equivalent prior to using Vue Test Utils while unit test should be using shallowMount in my opinion.

With this, the snapshots are going to change so I figured to change the snapshot serializer to using jest-serializer-vue instead as well.

Sample Carousel Snapshot:

<section class="VueCarousel">
  <div class="VueCarousel-wrapper">
    <div role="listbox" class="VueCarousel-inner" style="transform: translate(0px, 0); visibility: hidden; height: auto;"></div>
  </div>
  <pagination-stub></pagination-stub>
  <!---->
</section>

Please advise if you have anything to add.

[quinnlangille]

Hey @josephting, yes adding vue-test-utils was planned on our end already so it would be a welcomed addition! I think adding the snapshot serializer is a good idea as well, but let's do this incrementally as it will be a lot to review otherwise.

Let's make an initial PR adding utils, a second PR adding the new serializer, and any additional PRs after adding new test cases.

Sound reasonable?

[josephting]

Sounds good. I went ahead and fix the things that's currently broken as a first step.

Also, what do you think about Vue Testing Library? It's dom based testing using Dom Testing Library.

It would change the way we test the component though. As in, testing based on what we can see from DOM instead of knowing what specific methods to call etc. which is the implementation details.

Depends if you want finer control over testing to check specific lower level details in the code or as long as it'll work on the users' point of view.
DOM only testing approach may render code coverage of 100% unreachable.

For a component, I think code coverage of 100% is more important as developers are one of the user. Perhaps DOM Testing Library is catered towards an application and not component library.

Let me know what you think.

[quinnlangille]

I think both testing paradigms are valuable, rather than having one or the other. Ideally, Unit testing (what we currently have) is a check on individual methods and their input/output. Using the DOM testing lib and writing functional tests (what you're proposing) would be testing the functionality of the fully rendered carousel. Both can exist and be run as separate suites.

That being said, let's fix what we currently have before starting to add more. I'll take a look at your PR now :octocat:

[quinnlangille]

Hey @josephting, sorry for the delay. Just merged your PR into a new working branch, 0.17.0 which we should use for all of the testing improvements. Feel free to make further PRs against that branch!

[josephting]

Thanks for the notification, @quinnlangille. #335 is now ready for the next step.

[quinnlangille]

Just reviewed, will wait for a response from @ashleysimpson and if he has no input will merge right away. Thanks @josephting

[josephting]

Thank you, @quinnlangille.
I'll work on integrating jest-serializer-vue next.

• [x] Fix broken coverage tests (Jest configs)
• [x] [Vue Test Utils](https://github.com/vuejs/vue-test-utils) integration
• [x] Add jest-serializer-vue serializer
• [ ] Increase code coverage

[josephting]

@quinnlangille With all the prep work out of the way, we can begin to improve code coverage now.
To be honest, I'm not that experienced in coming up with test cases to address code coverage.

However, before we begin, can I know what's the objective here?

• Are we aiming for 100% coverage?
• Are we going to use vue-testing-library to do some of the unit tests?

[quinnlangille]

Yeah, correct @josephting, the aim is for 100% coverage (however, coverage percentage is not necessarily correlated to coverage quality, and we also want to keep that high). We're going to use vue-test-utils as much as we can, but the library is still in its infancy so we may need to get creative.

Inexperience is totally fine, that means you'll learn a lot from the project! Both @ashleysimpson and I have lots of experience writing test cases for vue, and we'll likely both contribute to the initiative as well. To start, I suggest we take care of the low hanging fruit, here is the current coverage report:

-----------------|----------|----------|----------|----------|-------------------|
File             |  % Stmts | % Branch |  % Funcs |  % Lines | Uncovered Line #s |
-----------------|----------|----------|----------|----------|-------------------|
All files        |    67.29 |    53.57 |    69.64 |    67.69 |                   |
 src             |    65.63 |    52.59 |    68.57 |    65.79 |                   |
  Carousel.vue   |    57.31 |    50.72 |    63.16 |    58.72 |... 70,573,593,621 |
  Navigation.vue |    90.91 |    57.14 |    85.71 |      100 |       33,44,45,46 |
  Pagination.vue |       88 |       75 |       80 |    88.24 |             15,26 |
  Slide.vue      |    88.89 |       44 |    83.33 |    87.18 |    23,66,67,69,70 |
 src/mixins      |    95.24 |       80 |    85.71 |    95.24 |                   |
  autoplay.js    |    95.24 |       80 |    85.71 |    95.24 |                56 |
-----------------|----------|----------|----------|----------|-------------------|

=============================== Coverage summary ===============================
Statements   : 67.29% ( 251/373 )
Branches     : 53.57% ( 150/280 )
Functions    : 69.64% ( 78/112 )
Lines        : 67.69% ( 220/325 )

To get a more detailed look, we can run yarn test-coverage which will generage the coverage folder in the project root. Inside, if you open coverage/client/index.html it will give you an interactive coverage report where you can go through line by line. On my output it seems the coverage highlighting is off (I don't think it likes the single file component), but you can still get a jist of what lines we're missing.

I suggest we start with covering the carosuel methods input/output and then we can move to the more granular conditional testing. What do you think?

Also, I trust your ability to write the test cases, but if you're more comfortable having them provided I can write out some out here if you'd like - just let me know! :octocat:

Also, thanks so much for all of your work on this so far - it's been a huge help and is in perfect timing with our planned v1.0.0 refactor 🚀

[josephting]

@quinnlangille I attempted to increase the code coverage but to no success. I guess I'm just not proficient enough to figure out what's going on behind the scene.

My attempt here is to test adjustableHeight and adjustableHeightEasing properties. From there, I can see that transitionStyle() computed value directly relates to the properties above.

So, I came up with 2 cases where: (assuming adjustableHeightEasing = 'linear' & speed = 0)

1. transitionStyle() should return 0s ease transform when adjustableHeight = false
2. transitionStyle() should return 0s ease transform, height 0s linear when adjustableHeight = true

Here are the tests I've written. Both of them can fail if I provide incorrect expected values and passes when correct expected values are given.

it('should not set adjustable height transition easing', () => {
  const wrapper = mount(Carousel, {
    propsData: {
      adjustableHeight: false,
      adjustableHeightEasing: 'linear',
      speed: 0
    }
  });

  expect(wrapper.vm.transitionStyle).toBe('0s ease transform');
});

it('should set adjustable height transition easing', () => {
  const wrapper = mount(Carousel, {
    propsData: {
      adjustableHeight: true,
      adjustableHeightEasing: 'linear',
      speed: 0,
      perPage: 1
    },
    slots: {
      default: [Slide, Slide, Slide]
    },
    sync: false
  });

  expect(wrapper.vm.transitionStyle).toBe('0s ease transform, height 0s linear');
});

However, this section just don't seem to get tracked by the coverage tool.
Carousel.vue coverage

When I console.log(wrapper.vm.$refs['VueCarousel-inner'].style), this is what I get. transition don't seems to get initialized and still doesn't when I check this in $nextTick.

CSSStyleDeclaration {
      '0': 'transform',
      '1': 'visibility',
      '2': 'height',
      _values:
       { transform: 'translate(0px, 0)',
         visibility: 'hidden',
         height: 'auto' },
      _importants:
       { transform: undefined,
         visibility: undefined,
         height: undefined },
      _length: 3,
      _onChange: [Function],
      undefined: '0px' }

By reading the code in Carousel.vue, I think I can find what to be tested but I just don't know how to get the coverage rate increased effectively.

[quinnlangille]

@josephting your test looks fine, given the case you're trying to test. This seems like an issue with the coverage tool, rather than your test. Maybe even some vue-test-utils weirdness considering the initialization issue.

If we can deterministically fail or pass the tests, and the snapshot output is what we're expecting - I'd consider that a win. Mind pushing up your branch and I'll take a look into the coverage reporting?