Unit tests for LibraryPage

[nucleogenesis]

Summary

Unit tests for LibraryPage - a collaboration between myself, @marcellamaki and @sairina

Some changes were made in the EmbeddedSidePanel/SelectGroup.vue file to clean-up jest output that didn't affect test success or failure.

The test suite runs in about 2.5s on my laptop - so I opted to leave the tests pretty verbose - shallowMount-ing everything local to each it() function. I hope that there being less indirection makes the test suite - or at least each individual test - easier to read and modify if/when the component's requirements change.

References

Fixes #8980

Reviewer guidance

• Do the test scenarios read in a way that seems to define the behavior of the component well?
• Is it clear in each test what is happening and why? Should there be any comments to clarify anything?
• Can you run the test suite locally and see how long it takes? - Not on the first run which seems to take much longer so hit Enter again after the first run through when you run test only running the library-page.spec.js file.

---

Testing checklist

• Contributor has fully tested the PR manually
• If there are any front-end changes, before/after screenshots are included
• Critical user journeys are covered by Gherkin stories
• Critical and brittle code paths are covered by unit tests

PR process

• PR has the correct target branch and milestone
• PR has 'needs review' or 'work-in-progress' label
• If PR is ready for review, a reviewer has been added. (Don't use 'Assignees')
• If this is an important user-facing change, PR or related issue has a 'changelog' label
• If this includes an internal dependency change, a link to the diff is provided

Reviewer checklist

• Automated test coverage is satisfactory
• PR is fully functional
• PR has been tested for accessibility regressions
• External dependency files were updated if necessary (yarn and pip)
• Documentation is updated
• Contributor is in AUTHORS.md

[MisRob]

Thank you, @nucleogenesis, @sairina, and @marcellamaki

One of the things that I'd suggest addressing are dependencies of the whole test suite on the internal implementation of the library page whenever it is technically possible, particularly in these two overlapping areas:

Descriptions of tests

Examples

• method: toggleSidePanelVisibility toggles this/vm.sidePanelIsOpen
• default view: when displayingSearchResults is falsy and there are rootNodes displays a ChannelCardGroupGrid
• displays coreString.overCertainNumberOfSearchResults with useSearch#results.length when useSearch#more is truthy
• when there is sidePanelContent, show FullScreenSidePanel
• displays a ChannelCardGroupGrid

One of the functions of the unit tests suite, as I understand it, is documenting the functionality and possible states of components being tested without the need for a comprehensive study of its internals. From such descriptions, it's not possible to understand what is the library page supposed to do. You can try it by forgetting all you know about the library page and reading the examples above. Another disadvantage is that if we rename a component or a method, a test description won't make sense anymore, resulting in more maintenance time on checking it, which is often forgotten and can easily cause confusion later on.

My suggestion would be to describe all tests from the user point of view, so instead of "method: toggleSidePanelVisibility toggles this/vm.sidePanelIsOpen " we'd rather have 'Filter' button click toggles the side panel"

EDIT (March 2) After discussing this with @nucleogenesis we figured out that one exception is names of methods and data coming from composables which is similar to using the third-party API, so using for example useSearch#more makes sense + documents dependencies to other layers of the app.

Testing internal data and methods

Example (source)

describe('method: toggleSidePanelVisibility', () => {
  it('toggles `this/vm.sidePanelIsOpen`', () => {
    const wrapper = shallowMount(LibraryPage, {
      localVue,
      store: mockStore,
    });
    expect(wrapper.vm.sidePanelIsOpen).toBeFalsy();
    wrapper.vm.toggleSidePanelVisibility();
    expect(wrapper.vm.sidePanelIsOpen).toBeTruthy();
  });
});

Testing internal methods and data has the following disadvantages:

• Brittle tests that don't inform us whether something works. In this example, the fact that wrapper.vm.sidePanelIsOpen is truthy doesn't need to mean that the side panel is open necessarily. We might, for example, use this variable in the template in the wrong way or not use it at all - the test will pass, and the side panel won't show.
• Such tests are basically reimplementing a component resulting in more maintenance time. Whenever we refactor components, we will need to refactor their tests even though user-facing behavior is still the same. Even just variable name change in a component would force us to go into its test suite and change it there too.

I can imagine some cases when it's useful to explicitly test internals, for example when we need to be 100% certain about a very complex and critical method.

For those reasons, I'd suggest writing tests from the user's perspective as much as possible. In the example below, it'd mean to simulate clicking the Filter button via KDS API and checking that the side panel is visible/invisible.

In cases of facing some complicated technical limitations, I think it'd be acceptable to fall back to reaching to internals, but I would recommend commenting on it.

EDIT (March 2) Exceptions:

• mocking KResponsiveWindowMixin (e.g. windowIsSmall computed property)

[MisRob]

To reply

Can you run the test suite locally and see how long it takes? - Not on the first run which seems to take much longer so hit Enter again after the first run through when you run test only running the library-page.spec.js file.

it is usually around 2.2 seconds for my local runs.

[nucleogenesis]

Thank you so much @MisRob !

I've pushed changes re: the example you shared, see my last commit for the direct changes there.

I have some questions about the rest that I think would be far easier to discuss on a call so I'll reach out separately to discuss and summarize the outcomes in this thread.

[nucleogenesis]

Thanks @MisRob - force merging per your note in Slack