-
Notifications
You must be signed in to change notification settings - Fork 274
[Remove Vuetify from Studio] Convert 'Reset password' unit tests to Vue Testing Library #5644
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: unstable
Are you sure you want to change the base?
[Remove Vuetify from Studio] Convert 'Reset password' unit tests to Vue Testing Library #5644
Conversation
Migrates test suite to Vue Testing Library to focus on user behavior rather than implementation details. - Replaced brittle wrapper manipulation with screen queries - Added validation and successful submission scenarios - Verified correct payload dispatch to Vuex with query params Closes learningequality#5637
|
👋 Thanks for contributing! We will assign a reviewer within the next two weeks. In the meantime, please ensure that:
We'll be in touch! 😊 |
|
Hi @LightCreator1007, thank you! Before I loop in maintainers, I will invite the community to review - you don't need to take the feedback as definitive - this first round is meant more as a collaborative process. Please engage and clarify together as needed :) |
|
Hi @LightCreator1007 , Hi @MisRob The migration to Vue Testing Library clearly shifts the tests toward user-observable behavior, which aligns well with VTL philosophy and reduces brittleness compared to wrapper-driven assertions. Using screen queries makes the intent of each test easier to read and reason about, especially for contributors who are less familiar with the component internals. The added coverage for both validation errors and successful submission improves confidence in the ResetPassword flow. Verifying the dispatched Vuex payload (including query params) feels appropriate here, since the store interaction is a critical part of the feature contract. Overall, the refactor keeps the test file focused and readable despite expanding scope — not an easy balance to strike. One small, forward-looking thought (not a blocker): Some assertions couple the test to the exact Vuex dispatch shape. That seems reasonable for this flow, but I was curious whether this was a deliberate choice to treat the store interaction as part of the public contract, or whether future VTL migrations might prefer asserting only on user-facing outcomes where possible. Either approach seems defensible here — just interested in the rationale. From my side, I didn’t spot any regressions or inconsistencies introduced by the migration. This looks consistent with the direction the project is taking, and I think it’ll serve as a good reference for similar test conversions going forward. Thanks again for pushing this forward, and happy to help review follow-ups if needed 🙂 |
|
Hi @LightCreator1007 , @MisRob — sharing a community review after reading through the diff and walking through the test flows end to end. I focused mainly on whether the new tests still exercise the full reset flow correctly after the VTU → VTL migration. The renderComponent(queryParams) helper makes the setup explicit, especially how the router is primed with query params before render, which helped me trace how those values end up in the Vuex dispatch. I stepped through the validation cases and confirmed the tests now rely entirely on user-visible behavior (updating inputs via labels, submitting via the button, and asserting on rendered messages), without reaching into component state or refs. That made it easier to reason about what would actually break if the UI changed. I also checked the async paths carefully — wrapping both the account/setPassword assertion and the route transition in waitFor avoids timing issues, and clearing mocks in beforeEach keeps the scenarios isolated from one another. The assertion on the dispatched payload (merging query params with the new passwords) looks intentional to me given how central that merge is to the reset flow. After comparing with the previous tests, I didn’t see any coverage gaps or behavior changes introduced by the refactor. |
|
Hllo @PixelCraftLab , @TheGreatPratyush . To address the specific question regarding the Vuex payload assertions:
|
|
Thanks for explaining that, @LightCreator1007 — I agree. With the Vuex action mocked, asserting on the dispatched payload is the right safeguard against regressions like dropping the token, which wouldn’t be caught by the UI success state alone. That rationale matches what I was checking for while reviewing the migration, specifically around preserving the reset-flow contract during the VTU → VTL refactor. |
Fixes #5637
Migrates test suite to Vue Testing Library to focus on user behavior rather than implementation details.