Skip to content
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

[Client Validation] Use emptyWidgetsFunctional in scoring #2083

Open
wants to merge 9 commits into
base: feature/client-validation
Choose a base branch
from
5 changes: 5 additions & 0 deletions .changeset/pink-pumas-hug.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@khanacademy/perseus": minor
---

Use empty widgets check in scoring function
163 changes: 142 additions & 21 deletions packages/perseus/src/renderer-util.test.ts
Original file line number Diff line number Diff line change
@@ -1,11 +1,12 @@
import {screen} from "@testing-library/react";
import {act, screen} from "@testing-library/react";
import {userEvent as userEventLib} from "@testing-library/user-event";

import {
testDependencies,
testDependenciesV2,
} from "../../../testing/test-dependencies";

import {question1} from "./__testdata__/renderer.testdata";
import * as Dependencies from "./dependencies";
import {
emptyWidgetsFunctional,
Expand All @@ -15,7 +16,7 @@ import {
import {mockStrings} from "./strings";
import {registerAllWidgetsForTesting} from "./util/register-all-widgets-for-testing";
import {renderQuestion} from "./widgets/__testutils__/renderQuestion";
import {question1} from "./widgets/group/group.testdata";
import DropdownWidgetExport from "./widgets/dropdown";

import type {
DropdownWidget,
Expand Down Expand Up @@ -78,6 +79,7 @@ function getLegacyExpressionWidget() {
},
};
}

describe("renderer utils", () => {
beforeAll(() => {
registerAllWidgetsForTesting();
Expand Down Expand Up @@ -743,23 +745,148 @@ describe("renderer utils", () => {
});
});

it("should return empty if any validator returns empty", () => {
// Act
const validatorSpy = jest
.spyOn(DropdownWidgetExport, "validator")
// Empty
.mockReturnValueOnce({
type: "invalid",
message: null,
})
// Not empty
.mockReturnValueOnce(null);
const scoringSpy = jest.spyOn(DropdownWidgetExport, "scorer");

Copy link
Contributor

Choose a reason for hiding this comment

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

From the perspective of an outsider, I like this strategy. From the perspective of someone actively working on splitting up WidgetExports, I wonder if there's a way to do these tests without spying on WidgetExports; for example we could get 90% there by creating an empty user input object and that would keep me from having to refactor this test when I make my changes.

No action needed, just something that came to mind.

// Act
const score = scorePerseusItem(
{
content:
question1.content +
question1.content.replace("dropdown 1", "dropdown 2"),
widgets: {
"dropdown 1": question1.widgets["dropdown 1"],
"dropdown 2": question1.widgets["dropdown 1"],
},
images: {},
},
{},
mockStrings,
"en",
);

// Assert
expect(validatorSpy).toHaveBeenCalledTimes(2);
expect(scoringSpy).not.toHaveBeenCalled();
expect(score).toEqual({type: "invalid", message: null});
});

it("should score item if all validators return null", () => {
// Arrange
const validatorSpy = jest
.spyOn(DropdownWidgetExport, "validator")
.mockReturnValue(null);
const scoreSpy = jest
.spyOn(DropdownWidgetExport, "scorer")
.mockReturnValue({
type: "points",
total: 1,
earned: 1,
message: null,
});

// Act
const score = scorePerseusItem(
{
content:
question1.content +
question1.content.replace("dropdown 1", "dropdown 2"),
widgets: {
"dropdown 1": question1.widgets["dropdown 1"],
"dropdown 2": question1.widgets["dropdown 1"],
},
images: {},
},
{"dropdown 1": {value: 0}},
mockStrings,
"en",
);

// Assert
expect(validatorSpy).toHaveBeenCalledTimes(2);
expect(scoreSpy).toHaveBeenCalled();
expect(score).toEqual({
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we also expect this to have been called 2 times? Would that be valuable to include?

type: "points",
total: 2,
earned: 2,
message: null,
});
});

it("should return correct, with no points earned, if widget is static", () => {
const validatorSpy = jest.spyOn(DropdownWidgetExport, "validator");

const score = scorePerseusItem(
{
...question1,
widgets: {
"dropdown 1": {
...question1.widgets["dropdown 1"],
static: true,
},
},
},
{"dropdown 1": {value: 1}},
mockStrings,
"en",
);

expect(validatorSpy).not.toHaveBeenCalled();
expect(score).toHaveBeenAnsweredCorrectly({
shouldHavePoints: false,
});
});

it("should ignore widgets that aren't referenced in content", () => {
const validatorSpy = jest.spyOn(DropdownWidgetExport, "validator");
const score = scorePerseusItem(
{
content:
"This content references [[☃ dropdown 1]] but not dropdown 2!",
widgets: {
...question1.widgets,
"dropdown 2": {
...question1.widgets["dropdown 1"],
},
},
images: {},
},
{"dropdown 1": {value: 2}},
mockStrings,
"en",
);

expect(validatorSpy).toHaveBeenCalledTimes(1);
expect(score).toHaveBeenAnsweredCorrectly({
shouldHavePoints: true,
});
});

it("should return score from contained Renderer", async () => {
// Arrange
const {renderer} = renderQuestion(question1);
// Answer all widgets correctly
await userEvent.click(screen.getAllByRole("radio")[4]);
// Note(jeremy): If we don't tab away from the radio button in this
// test, it seems like the userEvent typing doesn't land in the first
// text field.
await userEvent.tab();
await userEvent.type(
screen.getByRole("textbox", {name: /nearest ten/}),
"230",
);
await userEvent.type(
screen.getByRole("textbox", {name: /nearest hundred/}),
"200",

// Answer correctly
await userEvent.click(screen.getByRole("combobox"));
await act(() => jest.runOnlyPendingTimers());

await userEvent.click(
screen.getByRole("option", {
name: "less than or equal to",
}),
);

// Act
const userInput = renderer.getUserInputMap();
const score = scorePerseusItem(
question1,
Expand All @@ -770,12 +897,6 @@ describe("renderer utils", () => {

// Assert
expect(score).toHaveBeenAnsweredCorrectly();
expect(score).toEqual({
earned: 3,
message: null,
total: 3,
type: "points",
});
});
});
});
24 changes: 23 additions & 1 deletion packages/perseus/src/renderer-util.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,16 @@ import type {
ValidationDataMap,
} from "./validation.types";

function mapWidgetIdsToEmptyScore(widgetIds: ReadonlyArray<string>): {
[widgetId: string]: PerseusScore;
} {
const emptyResult: {[widgetId: string]: PerseusScore} = {};
widgetIds.forEach((widgetId) => {
emptyResult[widgetId] = {type: "invalid", message: null};
});
return emptyResult;
}

export function getUpgradedWidgetOptions(
oldWidgetOptions: PerseusWidgetsMap,
): PerseusWidgetsMap {
Expand Down Expand Up @@ -50,7 +60,7 @@ export function emptyWidgetsFunctional(
widgets: ValidationDataMap,
// This is a port of old code, I'm not sure why
// we need widgetIds vs the keys of the widgets object
widgetIds: Array<string>,
widgetIds: ReadonlyArray<string>,
userInputMap: UserInputMap,
strings: PerseusStrings,
locale: string,
Expand Down Expand Up @@ -108,6 +118,18 @@ export function scoreWidgetsFunctional(
): {[widgetId: string]: PerseusScore} {
const upgradedWidgets = getUpgradedWidgetOptions(widgets);

// Do empty check first
const emptyWidgets = emptyWidgetsFunctional(
widgets,
widgetIds,
userInputMap,
strings,
locale,
);
if (emptyWidgets.length > 0) {
return mapWidgetIdsToEmptyScore(emptyWidgets);
Copy link
Contributor

Choose a reason for hiding this comment

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

This returns from the whole scoreWidgetsFunctional function, right? EmptyWidgets only contains the widgets that are empty, right? What happens if there are non-empty widgets though? Do those get lost if we return when any of the widgets checked are empty?

}

const gradedWidgetIds = widgetIds.filter((id) => {
const props = upgradedWidgets[id];
const widgetIsGraded: boolean = props?.graded == null || props.graded;
Expand Down
Loading