Fix spec violations in ee/spec/frontend/test_case_show/components/test_case_sidebar_spec.js
Problem description
The spec file ee/spec/frontend/test_case_show/components/test_case_sidebar_spec.js
contains technical debt and/or antipatterns that need to be fixed. This is a prerequisite for updating our codebase to Vue3
.
Proposal
The following issues have been identified in this spec file:
- overriding method behavior
- spying on methods
-
.setData()
usage
Please see the related epic for more details on how these violations can be addressed.
Technical Proposal
In moving from Vue2 to Vue3, there are several testing patterns that exist in the code base that are bad practice and break on update the framework. These patterns must be removed before the update can occur.
This dashboard was created to track the instances that need to be updated.
Pattern 1: Testing Vue data (vm-access)
We should never verify that the data on a Vue instance has been updated, but instead follow the user and verify the the UI has updated accordingly.
Instead one should:
- find which component uses that data and test the props
Example 1
- expect(wrapper.vm.hasKeys).toEqual(3);
+ expect(findKeysDisplay().props('hasKeys')).toBe(3)
Pattern 2: Remove direct modification of Vue instances (vm-assign)
We should never treat data of Vue instances as it's public interface, so updating Vue instance data via assignment (e.g. wrapper.vm.foo = 5
) is invalid.
Instead one should:
- using
setProps
iffoo
is a prop - emulating user actions / system changes (e.g. dispatching actions, triggering / emitting events)
- as a last resort that should be avoided by all cost, use
setData
Example 1
- wrapper.vm.$refs.markdownDrawer.toggleDrawer = drawerToggleSpy;
Pattern 3: Remove spying on Vue components methods (vm-method-spy instances)
We should never treat methods of Vue instances as a public interface unless it is intended to be public (e.g. show & hide on modals). Calls similar to jest.spyOn(wrapper.vm, 'someMethod').mockImplementation()
are just another way to implement setMethods
, which is harmful and should be avoided.
Instead one should:
- track what the method does and how it impacts the UI or calls an API and test the UI changes or API calls
To ensure our tests properly check our components only in expected way such calls should be removed
Example 1
- beforeEach(() => { jest.spyOn(wrapper.vm, 'processAllReferences'); })
...
- expect(wrapper.vm.processAllReferences).toHaveBeenCalledWith(input);
+ expect(findRelatedIssuesBlock().props('pendingReferences')).toEqual([input]);
+ expect(putMethodMock).toHaveBeenCalledWith(`${TEST_ENDPOINT}/${input}`);
setData
calls and data()
as mount option in Vue testing suite (vtu-banned-method)
Pattern 4: Get rid of Direct setting of data on a Vue instance using setData
is generally an anti-pattern. It indicates that our tests rely too much on internal component structure, which could produce tests that pass with a UI that has a bug.
Instead one should:
- refactor tests to interact with component via "external means" (e.g. by triggering native/Vue events on child components or changing external data sources)
Example 1
- wrapper.setData({ selected: [item] })
+ dropdown.vm.$emit('selected', item)