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.
43 changes: 37 additions & 6 deletions packages/perseus/src/widgets/orderer/score-orderer.test.ts
Original file line number Diff line number Diff line change
@@ -1,12 +1,13 @@
import {question1} from "./orderer.testdata";
import {scoreOrderer} from "./score-orderer";
import * as OrdererValidator from "./validate-orderer";

import type {
PerseusOrdererRubric,
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 All @@ -21,7 +22,7 @@ describe("ordererValiator", () => {
// Act
const result = scoreOrderer(userInput, rubric);

// assert
// Assert
expect(result).toHaveBeenAnsweredCorrectly();
});

Expand All @@ -37,7 +38,7 @@ describe("ordererValiator", () => {
// Act
const result = scoreOrderer(userInput, rubric);

// assert
// Assert
expect(result).toHaveBeenAnsweredIncorrectly();
});

Expand All @@ -53,12 +54,41 @@ describe("ordererValiator", () => {
// Act
const result = scoreOrderer(userInput, rubric);

// assert
// 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.

it("should be correctly answerable if validation passes", () => {
// Arrange
const mockValidator = jest
.spyOn(OrdererValidator, "default")
.mockReturnValue(null);

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

const userInput: PerseusOrdererUserInput = {
current: question1.widgets["orderer 1"].options.correctOptions.map(
(option) => option.content,
),
};
// Act
const result = scoreOrderer(userInput, rubric);

// Assert
expect(mockValidator).toHaveBeenCalledWith(userInput);
expect(result).toHaveBeenAnsweredCorrectly();
});

it("should return an invalid response if validation fails", () => {
// Arrange
const mockValidator = jest
.spyOn(OrdererValidator, "default")
.mockReturnValue({
type: "invalid",
message: null,
});

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

Expand All @@ -69,7 +99,8 @@ describe("ordererValiator", () => {
// Act
const result = scoreOrderer(userInput, rubric);

// assert
// Assert
expect(mockValidator).toHaveBeenCalledWith(userInput);
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();
});
});
23 changes: 23 additions & 0 deletions packages/perseus/src/widgets/orderer/validate-orderer.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
import type {PerseusScore} from "../../types";
import type {PerseusOrdererUserInput} from "../../validation.types";

/**
* Checks user input from the orderer widget to see if the user has started
* ordering the options, making the widget scorable.
* @param userInput
* @see `scoreOrderer` for more details.
*/
function validateOrderer(
userInput: PerseusOrdererUserInput,
): Extract<PerseusScore, {type: "invalid"}> | null {
if (userInput.current.length === 0) {
return {
type: "invalid",
message: null,
};
}

return null;
}

export default validateOrderer;