WIP adding unit tests

[mikeu]

Starting work on expanding the test suite to address #345, with some of the helper functions in utils

[DonNicoJs]

@mikeu nice start! At a first look I have a tip: you may want to check jest timers handling which can make the tests faster (jest has an utility to 'advance time')

[mikeu]

Aha, that does sound useful, thanks @DonNicoJs!

[mikeu]

@DonNicoJs sorry for the extended silence, I've been out of town.

Do you have any thoughts on how best to mount Vue2Leaflet components for testing? Mounting in complete isolation doesn't work, because then there's no underlying map, so calls along the lines of this.mapObject.addTo(this.$parent.mapObject) in the components' mounted hooks fail because $parent.mapObject is undefined.

I've tried variations such as providing an LMap as the parentComponent, or mounting a separate LMap instance and providing the component under test as either a child or in a slot, or trying to mock the parent's mapObject property. These have had varying levels of success, but I have not yet found an approach that gives everything we'd need.

Ideally, we should be able to generate a wrapper object for a mounted component that allows setProps, setData, and the rest of the vue-test-utils wrapper API to work correctly, while also exposing a vm.mapObject so that the tests can determine whether changes to the component are having the expected effect on Leaflet. And hopefully the same approach will work without much modification for at least most of the Vue2Leaflet components to be tested.

I'll keep playing with this over the coming days, but if you have any ideas I'd be happy to hear them!

[bezany]

@mikeu I did it! See here #376
This example did very helpful

[DonNicoJs]

@bezany thank you for jumping in here! :)

[DonNicoJs]

@mikeu @bezany #376 merged, if you can collaborate on mikeu fork it would be great otherwise I am here to offer as much assistance as I can

[bezany]

@mikeu Oh, I didn't saw this commit. Approach with default slot for LMap look interesting and more elegant than usage created hook.
Ok, look like we found worked solution. May be in future we usage provide/inject and than we can use prodive options.
Also may be update @vue/test-utils to last version.

[mikeu]

@bezany yes, I was quite hopeful about using the LMap slot as well, but didn't find a good approach that allows interaction with the component's wrapper through setProps etc. after mounting, and also allows straightforward addition of mounting options to the inner component, such as passing slot content to the LMarker for its label.

I'm certainly open to alternative approaches if you find one. As long as it continues to provide a { wrapper, mapWrapper } object then existing tests should keep working without refactoring. Or with minimal changes, if we have to make getWrapperWithMap async for example, that might be fine.

As for updating @vue/test-utils, the latest beta was already used in package-lock.json, but I updated package.json to set that as the minimum requirement going forward.

[DonNicoJs]

@mikeu Hi! how is going here? do you want me to merge this for now ?

[mikeu]

@DonNicoJs sorry for the delay, things got busy for a bit there and then I ran into some more troubles around the fact that Leaflet thought jsdom didn't support SVG elements. Eventually tracked that issue down (see this commit) and am now putting together a few more tests. I'll see how much more I have time for this week, but will be travelling next week so probably we can merge whatever I get done by the weekend. I'll let you know how it goes.

[mikeu]

@DonNicoJs in putting some of these tests together, I've come across a few edge cases that seem to have bugs associated with them. For example, not being able to set a numeric property (e.g. fillOpacity) to 0 because that is falsey, so the Vue2Leaflet setter logic doesn't call the setter on the underlying mapObject. So far nothing particularly major, but I'm unsure how best to handle this.

Should I include failing tests for these cases here, and fix them in separate issues / PRs? Or is it OK to add minor fixes to this PR so that the tests all pass? (Or would you prefer a third approach?)

[bezany]

@DonNicoJs May be merge this big PR? It will be helpful for future pull requests, if need writes some tests.
Travis CI build faild because need fix here. Look like we can remove equality test (if (newVal === oldVal) return;) in set* methods, because vue not call watcher anywhere if value not change. And check if (newVal) contains bug, because newVal may be 0. May be remove this check or use compare with null and undefined.

[DonNicoJs]

@bezany let me merge this, then we can fix the wrong test

[mikeu]

@DonNicoJs yes, that was one of the tests I asked about back in May, where I wasn't sure whether you wanted this PR to include failing tests that were fixed elsewhere, or include fixes as well as new tests, or not include failing tests at all and handle them in separate issues & PRs...

Was waiting for some guidance on that before finishing too many more tests, but merging this at this point definitely seems like a good start. We can continue to work on new tests from here. With this merged, it might make sense to generally add failing tests and their fixes as individual PRs rather than part of a large batch of otherwise passing tests, would you agree?

[DonNicoJs]

@mikeu yes, let's proceed with smaller PR, I am sorry if I did not partecipate sufficiently here, bad time for my free time lately.

[mikeu]

@DonNicoJs no worries, I figured you were probably just busy or away. I haven't had much free time either, so I was fine leaving this alone for a while. Now that it's merged and we have a plan for future work, I'll try to find some time to work on a few smaller PRs from here on out!