Mock Widget for cleaning up Tests #2072
Conversation
SonicScrewdriver
changed the title
| @@ -235,6 +235,14 @@ export type PerseusTableRubric = { | |||
|
|
|||
| export type PerseusTableUserInput = ReadonlyArray<ReadonlyArray<string>>; | |||
|
|
|||
| export type PerseusMockWidgetRubric = { | |||
There was a problem hiding this comment.
Note to self: I should ensure these are ordered alphabetically
| @@ -330,6 +331,8 @@ export type MoleculeRendererWidget = WidgetOptions<'molecule-renderer', PerseusM | |||
| export type RefTargetWidget = WidgetOptions<'passage-ref-target', PerseusPassageRefTargetWidgetOptions>; | |||
| // prettier-ignore | |||
| export type VideoWidget = WidgetOptions<'video', PerseusVideoWidgetOptions>; | |||
| // prettier-ignore | |||
| export type MockWidget = WidgetOptions<'mock-widget', MockWidgetOptions>; | |||
There was a problem hiding this comment.
Note to self: I should ensure these are ordered alphabetically
npm Snapshot: PublishedGood news!! We've packaged up the latest commit from this PR (ce9ff1e) and published it to npm. You Example: yarn add @khanacademy/perseus@PR2072If you are working in Khan Academy's webapp, you can run: ./dev/tools/bump_perseus_version.sh -t PR2072 |
|
Size Change: -2 B (0%) Total Size: 1.27 MB
ℹ️ View Unchanged
|
| @@ -0,0 +1,38 @@ | |||
| import KhanAnswerTypes from "../../util/answer-types"; | |||
There was a problem hiding this comment.
This will probably end up getting moved, or possible removed, as part of the Server Side Scoring project - which is fine!
|
|
||
| /** | ||
| * This is a Mock Perseus widget, which is used for our various rendering tests | ||
| * across both Perseus and Webapp. It is a simple widget that renders an interactable |
There was a problem hiding this comment.
Can you remove the reference to webapp (private repo)? I think it's enough to say it's for unit testing (both internally and in consuming projects)?
| * | ||
| * Please use this widget for all tests that are not specifically testing the | ||
| * functionality of a particular widget, such as testing the rendering components. | ||
| * This allows us to more easily update our widget schemas without needing to |
There was a problem hiding this comment.
| * This allows us to more easily update our widget schemas without needing to | |
| * This allows us to more easily update our widget schemas and behaviour without needing to |
| arg2: string, | ||
| arg3?: () => unknown | null | undefined, | ||
| ) => void = (path, newValue, cb) => { | ||
| /* c8 ignore next */ |
There was a problem hiding this comment.
I think we can remove this comment. We originally added these comments in a few, select places where we couldn't (or chose not to) unit test a line or block of code... this comment prevents the block from counting towards code coverage. I don't think we'll be too worried about test coverage of this mock widget as a whole.
| getInputPaths: () => ReadonlyArray<ReadonlyArray<string>> = () => { | ||
| // The widget itself is an input, so we return a single empty list to | ||
| // indicate this. | ||
| /* c8 ignore next */ |
There was a problem hiding this comment.
| /* c8 ignore next */ |
|
|
||
| const styles = StyleSheet.create({ | ||
| widgetContainer: { | ||
| color: "red", |
There was a problem hiding this comment.
Good idea to make this kinda blatantly non-production!
There was a problem hiding this comment.
Nice. I like the idea of moving these together under this folder.
| // Act | ||
| const input = "40"; | ||
| const textbox = screen.getByRole("textbox"); | ||
| await userEvent.click(textbox); |
There was a problem hiding this comment.
You can remove the .click() call when using .type() (it does that for you unless explicitly disabled)
| @@ -223,6 +223,7 @@ export type { | |||
| CollinearTuple, | |||
| MathFormat, | |||
| InputNumberWidget, // TODO(jeremy): remove? | |||
| NumericInputWidget, | |||
There was a problem hiding this comment.
What is needing this export? In a perfect world, we wouldn't export any of our widgets and usage of them would all be funnelled through a Renderer.
| import type {PerseusRenderer} from "../../perseus-types"; | ||
|
|
||
| export default { | ||
| question: { | ||
| content: "[[☃ mock-widget 1]] [[☃ mock-widget 2]]", | ||
| images: {}, | ||
| widgets: { | ||
| "mock-widget 1": { | ||
| type: "mock-widget", | ||
| graded: true, | ||
| options: { | ||
| value: "5", | ||
| }, | ||
| }, | ||
| "mock-widget 2": { | ||
| type: "mock-widget", | ||
| graded: true, | ||
| options: { | ||
| value: "6", | ||
| }, | ||
| }, | ||
| }, | ||
| } as PerseusRenderer, | ||
| answerArea: { | ||
| calculator: false, | ||
| }, | ||
| hints: [] as ReadonlyArray<any>, | ||
| }; |
There was a problem hiding this comment.
Instead of casting the question to PerseusRenderer, can you type the entire thing as a PerseusItem using satisfies?
| import type {PerseusRenderer} from "../../perseus-types"; | |
| export default { | |
| question: { | |
| content: "[[☃ mock-widget 1]] [[☃ mock-widget 2]]", | |
| images: {}, | |
| widgets: { | |
| "mock-widget 1": { | |
| type: "mock-widget", | |
| graded: true, | |
| options: { | |
| value: "5", | |
| }, | |
| }, | |
| "mock-widget 2": { | |
| type: "mock-widget", | |
| graded: true, | |
| options: { | |
| value: "6", | |
| }, | |
| }, | |
| }, | |
| } as PerseusRenderer, | |
| answerArea: { | |
| calculator: false, | |
| }, | |
| hints: [] as ReadonlyArray<any>, | |
| }; | |
| import type {PerseusItem} from "../../perseus-types"; | |
| export default { | |
| question: { | |
| content: "[[☃ mock-widget 1]] [[☃ mock-widget 2]]", | |
| images: {}, | |
| widgets: { | |
| "mock-widget 1": { | |
| type: "mock-widget", | |
| graded: true, | |
| options: { | |
| value: "5", | |
| }, | |
| }, | |
| "mock-widget 2": { | |
| type: "mock-widget", | |
| graded: true, | |
| options: { | |
| value: "6", | |
| }, | |
| }, | |
| }, | |
| }, | |
| answerArea: { | |
| calculator: false, | |
| }, | |
| hints: [] as ReadonlyArray<any>, | |
| } satisfies PerseusItem; |
| static: true, | ||
| }, | ||
| }, | ||
| }); | ||
| const cb = jest.fn(); | ||
|
|
||
| // Act | ||
| act(() => renderer.setInputValue(["input-number 2"], "1000", cb)); | ||
| act(() => renderer.setInputValue(["mock-widget 2"], "1000", cb)); | ||
| act(() => jest.runOnlyPendingTimers()); |
There was a problem hiding this comment.
With a simpler widget, we may be able to remove these jest.runOnlyPendingTimers() calls. Might be worth a try to simplify these tests slightly.
Summary:
This work was done as part of the Numeric Input project.
This PR introduces a new Mock Widget to be used for any tests that doesn't require testing the capabilities of a specific widget. I have updated as many tests and/or testdatas as I could find, except for a few key situations:
I have also updated as many occurrences of any remaining uses of
input-numberin our tests tonumeric-input, in order to help smooth the path forward for the eventual return to the Input Number Conversion project.I have opted not to include the widget in our extra-widgets / general registration for now as it would require that I also create an editor for it, but a caveat of this approach means that this widget is not accessible for Webapp tests. I would be happy to hear any thoughts regarding this approach, as I wasn't sure if I should be globally registering this widget.
Issue: LEMS-2615
Test plan: