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

Orderer: Extract validation out of scoring #1869

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
5 changes: 5 additions & 0 deletions .changeset/sharp-radios-burn.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@khanacademy/perseus": minor
---

Split out validation function for the `orderer` widget. This can be used to check if the user has ordered at least one option, confirming the question is ready to be scored.
18 changes: 1 addition & 17 deletions packages/perseus/src/widgets/orderer/score-orderer.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import type {
PerseusOrdererUserInput,
} from "../../validation.types";

describe("ordererValiator", () => {
describe("scoreOrderer", () => {
it("is correct when the userInput is in the same order and is the same length as the rubric's correctOption content items", () => {
// Arrange
const rubric: PerseusOrdererRubric =
Expand Down Expand Up @@ -56,20 +56,4 @@ describe("ordererValiator", () => {
// assert
expect(result).toHaveBeenAnsweredIncorrectly();
});

it("is invalid when the when the user has not started ordering the options and current is empty", () => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it is worth retaining a unit test for the scorer that makes sure the score is "invalid" for this case. This ensures we're correctly calling and handling the result of the validator in the scorer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added this back :) Do you think all scoring functions should retain a test confirming it can handle validation results? Also, I remember hearing about how it's ideal to make sure tests don't overlap so that multiple tests don't fail as a result of one piece of code failing (for example, validation). Is that why you mocked this out in your PRs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated the test to mock the validation and added a test confirming scoring handles passing validation with validation mocked.

// Arrange
const rubric: PerseusOrdererRubric =
question1.widgets["orderer 1"].options;

const userInput: PerseusOrdererUserInput = {
current: [],
};

// Act
const result = scoreOrderer(userInput, rubric);

// assert
expect(result).toHaveInvalidInput();
});
});
12 changes: 6 additions & 6 deletions packages/perseus/src/widgets/orderer/score-orderer.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
import _ from "underscore";

import {validateOrderer} from "./validate-orderer";

import type {PerseusScore} from "../../types";
import type {
PerseusOrdererRubric,
Expand All @@ -10,16 +12,14 @@ export function scoreOrderer(
userInput: PerseusOrdererUserInput,
rubric: PerseusOrdererRubric,
): PerseusScore {
if (userInput.current.length === 0) {
return {
type: "invalid",
message: null,
};
const validateError = validateOrderer(userInput);
if (validateError) {
return validateError;
}

const correct = _.isEqual(
userInput.current,
_.pluck(rubric.correctOptions, "content"),
rubric.correctOptions.map((option) => option.content),
);

return {
Expand Down
31 changes: 31 additions & 0 deletions packages/perseus/src/widgets/orderer/validate-orderer.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
import {validateOrderer} from "./validate-orderer";

import type {PerseusOrdererUserInput} from "../../validation.types";

describe("validateOrderer", () => {
it("is invalid when the user has not started ordering the options and current is empty", () => {
// Arrange
const userInput: PerseusOrdererUserInput = {
current: [],
};

// Act
const result = validateOrderer(userInput);

// Assert
expect(result).toHaveInvalidInput();
});

it("is null when the user has started ordering the options and current has at least one option", () => {
// Arrange
const userInput: PerseusOrdererUserInput = {
current: ["$10.9$"],
};

// Act
const result = validateOrderer(userInput);

// Assert
expect(result).toBeNull();
});
});
15 changes: 15 additions & 0 deletions packages/perseus/src/widgets/orderer/validate-orderer.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
import type {PerseusScore} from "../../types";
import type {PerseusOrdererUserInput} from "../../validation.types";

export function validateOrderer(
userInput: PerseusOrdererUserInput,
): Extract<PerseusScore, {type: "invalid"}> | null {
if (userInput.current.length === 0) {
return {
type: "invalid",
message: null,
};
}

return null;
}