Skip to content

Conversation

@vtushar06
Copy link
Contributor

@vtushar06 vtushar06 commented Nov 5, 2025

Summary

Converted Content Library (CatalogList) unit tests from Vue Test Utils to Vue Testing Library, focusing on user-observable behavior and removing implementation detail testing.

Key Changes:

  • Replaced mount() with render() and Vue Testing Library semantic queries
  • Removed 2 unused mock functions (mockDownloadChannelsCSV, mockDownloadChannelsPDF)
  • Consolidated 4 duplicate tests into 2 focused tests (10 total tests, all passing)
  • Simplified test setup using real Vuex store (component uses mapActions/mapGetters)
  • All tests now check visible outcomes: text content, button state, navigation

Test Coverage (10 tests):

  • Initial load: Results display, download button visibility, search action
  • Selection mode: Enter/exit selection, toolbar visibility, selection count
  • Channel selection: Select-all checkbox and count display
  • Search/filtering: Query parameter changes trigger search, results update
  • Download workflow: Select button availability
  • Offline/loading states tested through button disabled state

Verification:

Ran npm test -- catalogList.spec.js
It passed all 15 tests

References

Closes: #5527

Reviewer guidance

Tests focus on rendered content + business logic (VTL pattern)
No internal state access (removed tests on excluded array, component methods)

@vtushar06 vtushar06 marked this pull request as ready for review November 5, 2025 10:28
@MisRob
Copy link
Member

MisRob commented Nov 7, 2025

Thank you @vtushar06, within next two weeks, we will assign a reviewer.

@vtushar06
Copy link
Contributor Author

Hii @MisRob, I made some changes as you mentioned in my other PR about testing user-observable behavior instead of implementation details.
Please let me know If any more changes are required.

@marcellamaki marcellamaki self-assigned this Nov 18, 2025
@vtushar06
Copy link
Contributor Author

@MisRob ,I applied the approach from Issue #5474 and systematically removed stubs that weren't needed.
The remaining 4 stubs are minimal and necessary:

  • ChannelItem: isolates component under test
  • Checkbox: needed for selection mode testing
  • BottomBar: needed for toolbar testing
  • CatalogFilters: minimal stub

Tests now focus on user-visible behavior, Now PR is ready for review.

@vtushar06 vtushar06 requested a review from MisRob November 20, 2025 04:08
vtushar06 added a commit to vtushar06/studio that referenced this pull request Nov 20, 2025
…n from unstable

The contributor's StudioChip implementation is identical to ours.
We'll use their version from unstable and focus on test refactoring
following Issue learningequality#5536 patterns (data-present vs data-absent scenarios).

Following maintainer guidance from PR learningequality#5540 review.
@MisRob
Copy link
Member

MisRob commented Nov 24, 2025

@vtushar06

  • Stubs that you added because of data-testid are not needed - just add test ID on actual components. Not that it'd be wrong, but it will be simpler approach for now, and more aligned with other tests.
  • As for checkbox stub, you're basically re-implementing some parts of checkbox logic in your stub, and then based on it test that everything works well. But, in actual user experience, this implementation is not used. So again, this results in tests that actually don't reflect on what's really happening. Instead, just click the actual checkbox as user.

Stubs can be useful on some occasions, but I would generally recommend that until you get a grasp on them, it will be simpler if you always attempt not to use them at all. Only add a stub after you tried all other approaches, or if not using a stub in results in more complex or significantly slower tests. In majority of cases, you won't need them.

@vtushar06
Copy link
Contributor Author

Hi @MisRob,
Updated tests to use semantic queries (getByText) instead of test IDs. Can we move forward with this, as I have gone through merged PRs from their I thought this can be good approach for out changes.

@vtushar06
Copy link
Contributor Author

Hii @MisRob, This PR is now ready for review, please assign someone to review this and let me know If any more changes required.

Copy link
Member

@marcellamaki marcellamaki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @vtushar06 - thanks for your work on this! You've made a good start (and I can see that you've already applied some feedback from Misha regarding simplification and removing stubs) on a challenging task, and much of the work here is looking good :)

I know that in some other conversations with Misha, you've discussed the value of using getByText to confirm the rendering something in the DOM, and that can be a really useful approach. Here, however, you've added a mock for the translations, which I think adds unnecessary complexity, and potentially also brittleness. I added one particular inline comment just for more detail.

Could you try an approach that uses either strings without having to mock them, or another simple check to confirm rendering? That will help keep the test suite as simple as possible.

Two ideas for how to think about it, and you may want to apply these differently to different scenarios:

  • Is the string specifically what needs to be tested that it's rendered? If so, is there a way to do so without a mock?
  • If the string is a "proxy" of some type of display or changed state in the UX, is there another way to confirm that a particular (div, button, etc.) is rendered in a way that wouldn't require mocking a string? (this might require for example adding a test id on a component, and then checking that instead).

Misha and I will both be away starting next week for end of year holidays, but I will be happy to re-review or answer any more questions in January. Happy new year!

Copilot AI review requested due to automatic review settings December 19, 2025 07:52
@vtushar06 vtushar06 force-pushed the fix/content-vue-testing-library branch from 71af890 to b3c1b9c Compare December 19, 2025 07:52
@vtushar06
Copy link
Contributor Author

Hi @marcellamaki, Thank you for the detailed feedback

I've updated the tests to remove the translation mocks entirely. Now the tests use regex patterns to match the actual rendered text from the component's $trs translations:

Changes I made Removed all $tr mocks and Using screen.getByText(/results found/i) to match actual rendered text
The component already translates text via $tr(), so the tests now verify the actual rendered output without any mocking complexity.

Ready for re-review when you're back in January. Happy holidays! 🎉

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR converts the Content Library (CatalogList) unit tests from Vue Test Utils to Vue Testing Library, shifting from implementation-detail testing to user-observable behavior testing. The migration simplifies the test setup by using a real Vuex store and consolidates duplicate tests.

Key Changes:

  • Replaced Vue Test Utils mount() with Vue Testing Library render() and semantic queries
  • Consolidated duplicate tests from 15 to 10 focused tests
  • Removed unused mock functions (downloadChannelsCSV, downloadChannelsPDF)

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Remove Vuetify from Studio] Convert Content Library unit tests to Vue Testing Library

3 participants