Convert hints editor unit tests to Vue Testing Library#5842
Convert hints editor unit tests to Vue Testing Library#5842Swoyamjeetcodes wants to merge 1 commit intolearningequality:unstablefrom
Conversation
|
👋 Hi @Swoyamjeetcodes, thanks for contributing! For the review process to begin, please verify that the following is satisfied:
Also check that issue requirements are satisfied & you ran Pull requests that don't follow the guidelines will be closed. Reviewer assignment can take up to 2 weeks. |
|
📢✨ Before we assign a reviewer, we'll turn on |
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
Clean migration — all acceptance criteria from #5815 met.
CI passing. Phase 3 skipped (no UI files changed). Two of the three PR videos (before/after recordings) failed to download — their S3 pre-signed URLs appear to have expired; only the test results video is accessible.
- praise: see inline — thorough open-index tracking coverage
- nitpick: see inline —
toBeDefined()inclickToolbarAction
@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly
How was this generated?
Reviewed the pull request diff checking for:
- Correctness: bugs, edge cases, undocumented behavior, resource leaks, hardcoded values
- Design: unnecessary complexity, naming, readability, comment accuracy, redundant state
- Architecture: duplicated concerns, minimal interfaces, composition over inheritance
- Testing: behavior-based assertions, mocks only at hard boundaries, accurate coverage
- Completeness: missing dependencies, unupdated usages, i18n, accessibility, security
- Principles: DRY (same reason to change), SRP, Rule of Three (no premature abstraction)
- Checked CI status and linked issue acceptance criteria
- For UI changes: inspected screenshots for layout, visual completeness, and consistency
| .trigger('click'); | ||
| const clickToolbarAction = async ({ action, hintIdx, user }) => { | ||
| const buttons = screen.getAllByTestId(`toolbarIcon-${action}`); | ||
| expect(buttons[hintIdx]).toBeDefined(); |
There was a problem hiding this comment.
nitpick: toBeDefined() is a plain Jest matcher. Since buttons[hintIdx] is a DOM node, toBeInTheDocument() is more idiomatic Testing Library and produces a clearer failure message if the button isn't found at the expected index.
|
📢✨ Before we assign a reviewer, we'll invite community pre-review. See the community review guidance for both authors and reviewers. |
MisRob
changed the title
AlexVelezLl
left a comment
AlexVelezLl
left a comment
There was a problem hiding this comment.
Thanks a lot for your contribution @Swoyamjeetcodes! Just a couple of things we may want to change to better align with new testing conventions in the org. Thanks!
| configure({ | ||
| testIdAttribute: 'data-test', | ||
| }); |
There was a problem hiding this comment.
@MisRob, I know we agreed on using data-testid for this, but I see that if we want to change this here, this will create a domino effect because there are tests relying on components like AssessmentItemToolbar that are also tested on other test suites, and we may end up modifying too many files for this. Is it okay if we leave it like this for now, and then, when all these related components are migrated, make the change to data-testid?
| expect(screen.getByRole('button', { name: 'New hint' })).toBeInTheDocument(); | ||
| }); | ||
|
|
||
| it('renders a placeholder when there are no hints', () => { | ||
| wrapper = mount(HintsEditor, { | ||
| propsData: { | ||
| hints: [], | ||
| }, | ||
| it('shows an empty-state message when a question has no hints', () => { | ||
| renderComponent({ | ||
| hints: [], | ||
| }); | ||
|
|
||
| expect(wrapper.html()).toContain('Question has no hints'); | ||
| expect(screen.getByText('Question has no hints')).toBeInTheDocument(); | ||
| }); | ||
|
|
||
| it('renders all hints in a correct order', () => { | ||
| wrapper = mount(HintsEditor, { | ||
| propsData: { | ||
| hints: [ | ||
| { hint: 'First hint', order: 1 }, | ||
| { hint: 'Second hint', order: 2 }, | ||
| ], | ||
| }, | ||
| it('shows hints in the same order as the question', () => { | ||
| renderComponent({ | ||
| hints: [ | ||
| { hint: 'First hint', order: 1 }, | ||
| { hint: 'Second hint', order: 2 }, | ||
| ], | ||
| }); | ||
|
|
||
| // Find all instances of your new RichTextEditor component | ||
| const editors = wrapper.findAllComponents({ name: 'RichTextEditor' }); | ||
| expect(editors.length).toBe(2); | ||
|
|
||
| // Instead of checking the raw HTML, we check the `value` prop passed to each editor. | ||
| expect(editors.at(0).props('value')).toBe('First hint'); | ||
| expect(editors.at(1).props('value')).toBe('Second hint'); | ||
| const hintCards = getHintCards(); | ||
| expect(within(hintCards[0]).getByText('First hint')).toBeInTheDocument(); | ||
| expect(within(hintCards[1]).getByText('Second hint')).toBeInTheDocument(); |
There was a problem hiding this comment.
Could we use the translation keys instead of the hardcoded strings for labels, please? So, instead of
screen.getByRole('button', { name: 'New hint' })We could do
screen.getByRole('button', { name: HintsEditor.$trs.newHintBtnLabel })| const MockTipTapEditor = { | ||
| name: 'TipTapEditor', | ||
| props: { | ||
| value: { | ||
| type: String, | ||
| default: '', | ||
| }, | ||
| mode: { | ||
| type: String, | ||
| default: 'view', | ||
| }, | ||
| }, | ||
| template: ` | ||
| <div> | ||
| <p v-if="value">{{ value }}</p> | ||
| <button | ||
| v-if="mode === 'edit'" | ||
| type="button" | ||
| aria-label="Update hint text" | ||
| @click="$emit('update', 'Updated hint')" | ||
| > | ||
| Update hint text | ||
| </button> | ||
| </div> | ||
| `, |
There was a problem hiding this comment.
I imagine this would be useful for other tests, too. Could you please create the mock on contentcuration/contentcuration/frontend/shared/views/TipTapEditor/TipTapEditor/__mocks__/TipTapEditor.vue and then just mock it with jest.mock('shared/views/TipTapEditor/TipTapEditor/TipTapEditor.vue');
For this mock's implementation, let's do it in a way that's closer to the actual implementation, like having this expose only a textarea element that emits the update event on change. Then we can just query this textarea, and use user.type to get the update events :). This way, we don't need to create elements like this button that may be confusing.
|
Hi @Swoyamjeetcodes! Could you please take a look at the feedback provided by @AlexVelezLl? Also, do let us know if you are unable to. Thank you! |
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
Prior findings
Unaddressed (re-raised below):
toBeDefined()vstoBeInTheDocument()inclickToolbarAction(nitpick) — no reply, code unchanged atHintsEditor.spec.js:57
0/1 prior findings resolved. 1 re-raised below.
CI has two failures — Python unit tests and Build all message files — both failing at "Install python dependencies with uv". The merge commit (90cf90d) left unresolved conflict markers in requirements.in and requirements.txt, which prevents uv from parsing those files. These files are also out of scope for this PR. Fix: rebase cleanly onto unstable, or resolve the conflicts by accepting the unstable side of both files.
No UI files changed — Phase 3 skipped.
Blocking: Unresolved merge conflicts in requirements.in and requirements.txt — see inline comments.
Nitpick (re-raised): toBeDefined() vs toBeInTheDocument() at HintsEditor.spec.js:57.
@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly
How was this generated?
Compared the current PR state against findings from a prior review:
- Retrieved prior bot reviews deterministically via the GitHub API
- Classified each prior finding as RESOLVED, UNADDRESSED, ACKNOWLEDGED, or CONTESTED
- Only raised NEW findings for newly introduced code
- Reviewed the pull request diff checking for correctness, design, architecture, testing, completeness, and adherence to DRY/SRP principles
- Checked CI status and linked issue acceptance criteria
| psycopg2-binary==2.9.11 | ||
| django-js-reverse==0.10.2 | ||
| django-registration==3.4 | ||
| <<<<<<< HEAD |
There was a problem hiding this comment.
blocking: This file has unresolved merge conflict markers committed by the merge commit (90cf90d). requirements.in is out of scope for this PR — it should not have been modified at all. Please rebase cleanly onto unstable (preferred), or resolve the conflict by keeping the unstable version of both le-utils and gunicorn and removing the conflict markers from this file and requirements.txt.
| latex2mathml==3.78.1 | ||
| # via -r requirements.in | ||
| <<<<<<< HEAD | ||
| le-utils==0.2.17 |
There was a problem hiding this comment.
blocking: Same unresolved merge conflict as requirements.in — conflict markers here prevent uv from parsing this file, causing the "Install python dependencies with uv" CI step to fail.
| .trigger('click'); | ||
| const clickToolbarAction = async ({ action, hintIdx, user }) => { | ||
| const buttons = screen.getAllByTestId(`toolbarIcon-${action}`); | ||
| expect(buttons[hintIdx]).toBeDefined(); |
There was a problem hiding this comment.
nitpick (re-raised from prior review): getAllByTestId already throws if no matching elements exist, so buttons[hintIdx] being present in the returned array is sufficient. Prefer expect(buttons[hintIdx]).toBeInTheDocument() — it's the idiomatic Testing Library assertion and produces a clearer failure message than toBeDefined().
Swoyamjeetcodes
force-pushed
the
chore/5789-hintseditor-vtl
branch
from
90cf90d to
04dfcf0
Compare
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
Prior findings
2/3 prior findings resolved since last review.
Newly resolved:
- Unresolved merge conflicts in
requirements.in(blocking) ← was UNADDRESSED — file now clean, CI passing - Unresolved merge conflicts in
requirements.txt(blocking) ← was UNADDRESSED — file now clean, CI passing
Still unaddressed (re-raised below):
toBeDefined()vstoBeInTheDocument()inclickToolbarAction(nitpick) — no reply, code unchanged atHintsEditor.spec.js:57
2/3 prior findings resolved. 1 re-raised below.
CI passing. No UI files changed — Phase 3 skipped.
- nitpick (re-raised):
toBeDefined()inclickToolbarAction— see inline
@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly
How was this generated?
Compared the current PR state against findings from a prior review:
- Retrieved prior bot reviews deterministically via the GitHub API
- Classified each prior finding as RESOLVED, UNADDRESSED, ACKNOWLEDGED, or CONTESTED
- Only raised NEW findings for newly introduced code
- Reviewed the pull request diff checking for correctness, design, architecture, testing, completeness, and adherence to DRY/SRP principles
- Checked CI status and linked issue acceptance criteria
| .trigger('click'); | ||
| const clickToolbarAction = async ({ action, hintIdx, user }) => { | ||
| const buttons = screen.getAllByTestId(`toolbarIcon-${action}`); | ||
| expect(buttons[hintIdx]).toBeDefined(); |
There was a problem hiding this comment.
nitpick (re-raised): getAllByTestId already throws if no matching elements exist, so buttons[hintIdx] being in the returned array is guaranteed. Prefer expect(buttons[hintIdx]).toBeInTheDocument() — it's the idiomatic Testing Library assertion and produces a clearer failure message than toBeDefined().
5 participants
Fixes #5815
Summary
Refactored
channelEdit/components/HintsEditor/HintsEditor.spec.jsto Vue Testing Library and rewrote the suite to reflect user interactions.What changed
@vue/test-utilsto:@testing-library/vue(render,screen,within)@testing-library/user-eventrenderComponenthelper (with router + component stubs).Manual verification
pnpm test contentcuration/contentcuration/frontend/channelEdit/components/HintsEditor/HintsEditor.spec.jsTest Suites: 1 passedTests: 12 passedUI evidence
Screen recording (Hints workflow in Questions tab)
Passed test cases
testcases.mp4
References
Reviewer guidance
contentcuration/contentcuration/frontend/channelEdit/components/HintsEditor/HintsEditor.spec.jspnpm test contentcuration/contentcuration/frontend/channelEdit/components/HintsEditor/HintsEditor.spec.jsAI usage
I used Codex (GPT-5) to help migrate the test suite and draft this PR description.
I critically reviewed and edited the generated output to match project testing conventions, removed unnecessary/legacy VTU patterns, and verified correctness by rerunning the migrated Jest file until all tests passed.