Convert hints editor unit tests to Vue Testing Library#5842
Convert hints editor unit tests to Vue Testing Library#5842Swoyamjeetcodes wants to merge 3 commits 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
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
…use translation keys - Replace local TipTapEditor mock in HintsEditor.spec.js with jest.mock to shared/views/TipTapEditor/TipTapEditor/TipTapEditor.vue - Create proper mock in shared/views/TipTapEditor/TipTapEditor/__mocks__/TipTapEditor.vue - Use translation keys instead of hardcoded strings for labels: * HintsEditor..newHintBtnLabel for 'New hint' button * HintsEditor..noHintsPlaceholder for empty state message - Mock exposes textarea element for better testing with user.type
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
Prior findings
2 prior findings unchanged (2 resolved, 0 acknowledged).
Still unaddressed (re-raised below):
toBeDefined()vstoBeInTheDocument()inclickToolbarAction(nitpick) — no reply, no code change
2/3 prior findings resolved. 1 re-raised below.
CI is failing. The new commit (63169e6da) introduced two regressions: the inline mock in HintsEditor.spec.js breaks the getByText() queries in that file's own tests; the new __mocks__/TipTapEditor.vue file breaks three other test files (AnswersEditor, AssessmentEditor, AssessmentItemEditor) that previously relied on auto-mocking. See inline comments.
- blocking (×2): inline mock uses textarea (breaks
getByText); shared__mocks__is hints-specific (breaks 3 other test files) - nitpick (re-raised):
toBeDefined()atHintsEditor.spec.js:52
@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
| name: 'TipTapEditor', | ||
| template: ` | ||
| <div> | ||
| <textarea |
There was a problem hiding this comment.
blocking: The inline mock renders the value prop inside a <textarea>. Testing Library's getByText() traverses DOM text nodes — it does not read a form control's .value. As a result, within(hintCards[0]).getByText('First hint') fails (CI: "Unable to find an element with the text: First hint"), and the button lookup in the update-text test fails because this template has no button element at all.
Fix: render value as a text node and add the button the tests expect. The __mocks__/TipTapEditor.vue file added in this commit already does this correctly. The simplest path is to remove the factory argument from this jest.mock() call — jest.mock('shared/views/TipTapEditor/TipTapEditor/TipTapEditor.vue') with no factory — so Jest falls through to the __mocks__ directory. If you prefer to keep the inline factory, change the template to <p v-if="value">{{ value }}</p> and add the button from the __mocks__ file.
| <button | ||
| v-if="mode === 'edit'" | ||
| type="button" | ||
| aria-label="Update hint text" |
There was a problem hiding this comment.
blocking: aria-label="Update hint text" and the hardcoded $emit('update', 'Updated hint') on line 9 are hints-specific. Placing this file in __mocks__/ makes it the automatic mock for any test that calls jest.mock('shared/views/TipTapEditor/TipTapEditor/TipTapEditor.vue') without a factory. AnswersEditor.spec.js, AssessmentItemEditor.spec.js, and AssessmentEditor.spec.js all do this — they were previously receiving a generic auto-mock and now get this hints-specific one instead, causing three additional CI failures.
Fix: either (a) delete this file — the inline factory in HintsEditor.spec.js is sufficient for that one test suite and restores the previous auto-mock behavior for the other three files — or (b) make it truly generic by removing the <button> and its hardcoded values, and keep the <p>{{ value }}</p> display only.
| .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 present in the 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.