replaced global vue with test vue in unit tests

[Nerdinacan]

Noticed a lot of client-side tests are continuing to use global Vue instead of our curated Vue instance.

Right now, it's just an annoyance with the eye-spam during test runs with the BootstrapVue components blithely trying to access a non-existent document.

But the problem is that component authors are not using the standard mount function which means they are unit testing in a Vue environment that is unlikely to match the one in which the component actually runs.

The helper we wrote is intended to generate a Vue instance that matches the one in the client in terms of plugins, filters, directive overrides, etc.

[dannon]

@Nerdinacan Thanks; I mentioned I was going to take care of this in #11121, but not having to do it myself (again) is even better!

[Nerdinacan]

Ahh crap sorry Dannon, didn't see that one.

[dannon]

Totally fine -- again, one less thing I have to do, and I'm not upset about that :)

[jmchilton]

If we're using that pattern over and over - does it make sense to have a wrapper around shallowMount in jest/helper that takes care of that detail (defaulting vue to that local vue I mean).

Update: Or maybe that is what mountRenderless is 😆 .

[dannon]

@jmchilton Yeah, when I added the helper my thinking it should be used sparingly, and was an extra indicator that a component was leaning on these extra directives. I'll admit the behavior is more prevalent than I was thinking though.

[Nerdinacan]

@jmchilton
Mount renderless is for the renderless components (i.e. providers). The ones with just a single big slot and you still need to check the slotProps even though there's nothing to render.

I think the only reason I didn't make a helper as you suggested is that we should really be using shallowMount, and a lot of people are using mount(), and I'd have to rewrite a bunch of tests to make that fly.

The fact that people are using mount() instead of shallowMount() tells me that the components are too monolithic.
Presumably, nobody cares whether b-form-checkbox properly prints a label, right? In a unit test, what you care about is probably that variable XYZ is true or false.

If all you care about is the variable you don't need a full mount(), you just check to see if wrapper.vm.XYZ is what you want after you set up the test?

I think the real problem is people are building big-ass components instead of breaking them into logical chunks. It's a common problem with new component designers who aren't yet comfortable with props in / events out.

[jmchilton]

The fact that people are using mount() instead of shallowMount() tells me that the components are too monolithic.

While I've switch to shallowMount more and more for my recent tests, the thing I would read into this is that nearly all the examples in the official docs ( https://vue-test-utils.vuejs.org/api/wrapper/#emitted, https://vue-test-utils.vuejs.org/api/wrapper/#methods, etc...) use mount instead of shallowMount.

[dannon]

@Nerdinacan RefactorConfirmationModal is still spewing to info in my quick test

[Nerdinacan]

@jmchilton
No argument there.

The official docs, and every video I've ever seen are basically teaching you how to write a unit test that is testing rendering. They are showing you how to write a unit test that is really more of a selenium test.

While that's important if you are indeed writing a new kind of low-level rendering component, what's more important and flexible is making sure the business logic works right since presumably the rendering should be able to change when somebody makes it prettier without busting tests.

I agree with you that the availability of good advice about writing tests is thin.

[Nerdinacan]

@dannon
Hmmm yeah, flushPromises()?
Or maybe lots of stuff in that folder does a Vue.use(). I'll look at that later.

[jmchilton]

I think we should just merge as is, seems atomic and working and just follow up with additional fixes in new PRs. I'll wait on the other requested reviewers though.

[dannon]

It's most of the way there, let's give @Nerdinacan the chance to address the last bit in this PR before merging.

[Nerdinacan]

@dannon
The problem is not the test, it's the code.

The message itself is an indicator that something strange is happening, and sure enough if you look at one of the data services used in that component, it references global Vue, seemingly to do the worrk of a Promise.

The Workflow editor needs to be fixed, not the test.

You can make the message disappear by setting the production flags on/off there, but the real problem is that Vue is being used at all in that file when it should just be an asynchronous function of some sort.

Vue.config.devtools = false

[Nerdinacan]

I went ahead and removed the suppression of the messages so we remember to refactor that service.

[github-actions]

This PR was merged without a "kind/" label, please correct.