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

Add and pass more regression tests for PerseusItem parser #1924

Merged
merged 22 commits into from
Dec 3, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/tiny-feet-give.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@khanacademy/perseus": patch
---

Internal: add and pass regression tests for PerseusItem parser's handling of legacy data
Original file line number Diff line number Diff line change
Expand Up @@ -565,3 +565,60 @@ exports[`renderer snapshots initial render: initial render 1`] = `
</div>
</div>
`;

exports[`renderer snapshots renders a placeholder for a deprecated widget: deprecated widget 1`] = `
<div>
<div
class="perseus-renderer perseus-renderer-responsive"
>
<div
class="paragraph"
data-perseus-paragraph-index="0"
>
<div
class="paragraph"
>
<div
class="perseus-widget-container widget-nohighlight widget-block"
>
<div
style="padding-top: 8px; padding-bottom: 8px;"
>
<div
class="default_xu2jcg-o_O-containerOuter_xu9ezy-o_O-inlineStyles_1ipey8s"
role="status"
>
<div
class="default_xu2jcg-o_O-backgroundColor_10s9e9d-o_O-inlineStyles_14jsc6v"
/>
<div
class="default_xu2jcg-o_O-containerInner_mm2xcu"
>
<span
aria-label="info"
class="svg_1q6jc65-o_O-icon_1521uiv-o_O-icon_16tp01m-o_O-inlineStyles_94g682"
data-testid="banner-kind-icon"
role="img"
/>
<div
class="default_xu2jcg-o_O-labelAndButtonsContainer_14rogkt"
>
<div
class="default_xu2jcg-o_O-labelContainer_1fjq5ke"
>
<span
class="text_f1191h-o_O-LabelSmall_hl4zs1"
>
Sorry, this part of the question is no longer available. 😅 Don't worry, you won't be graded on this part. Keep going!
</span>
</div>
</div>
</div>
</div>
</div>
</div>
</div>
</div>
</div>
</div>
`;
32 changes: 31 additions & 1 deletion packages/perseus/src/__tests__/renderer.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ import {simpleGroupQuestion} from "../widgets/group/group.testdata";
import InputNumberExport from "../widgets/input-number";
import RadioWidgetExport from "../widgets/radio";

import type {DropdownWidget} from "../perseus-types";
import type {PerseusRenderer, DropdownWidget} from "../perseus-types";
import type {APIOptions} from "../types";
import type {UserEvent} from "@testing-library/user-event";

Expand Down Expand Up @@ -107,6 +107,36 @@ describe("renderer", () => {
// Assert
expect(container).toMatchSnapshot("incorrect answer");
});

it("renders a placeholder for a deprecated widget", () => {
// Arrange
const question: PerseusRenderer = {
content: "[[☃ sequence 1]]",
images: {},
widgets: {
"sequence 1": {
type: "deprecated-standin",
version: {major: 0, minor: 0},
graded: true,
options: {
json: [
{
content: "",
images: {},
widgets: {},
},
],
},
},
},
};

// Act
const {container} = renderQuestion(question);

// Assert
expect(container).toMatchSnapshot("deprecated widget");
});
});

describe("linting (TranslationLinter)", () => {
Expand Down
14 changes: 8 additions & 6 deletions packages/perseus/src/perseus-types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,8 @@ export type PerseusWidgetsMap = {
[key in `table ${number}`]: TableWidget;
} & {
[key in `video ${number}`]: VideoWidget;
} & {
[key in `sequence ${number}`]: AutoCorrectWidget;
};

/**
Expand Down Expand Up @@ -287,7 +289,7 @@ export type RefTargetWidget = WidgetOptions<'passage-ref-target', PerseusPassage
// prettier-ignore
export type VideoWidget = WidgetOptions<'video', PerseusVideoWidgetOptions>;
//prettier-ignore
export type AutoCorrectWidget = WidgetOptions<'deprecated-standin', PerseusWidgetOptions>;
export type AutoCorrectWidget = WidgetOptions<'deprecated-standin', object>;

export type PerseusWidget =
| CategorizerWidget
Expand Down Expand Up @@ -633,10 +635,10 @@ export type PerseusInteractiveGraphWidgetOptions = {
step: [number, number];
// Where the grid lines on the graph will render. default [1, 1]
// NOTE(kevinb): perseus_data.go defines this as Array<number>
gridStep: [number, number];
gridStep?: [x: number, y: number];
// Where the graph points will lock to when they are dragged. default [0.5, 0.5]
// NOTE(kevinb): perseus_data.go defines this as Array<number>
snapStep: [number, number];
snapStep?: [x: number, y: number];
// An optional image to use in the background
backgroundImage?: PerseusImageBackground;
/**
Expand All @@ -647,7 +649,7 @@ export type PerseusInteractiveGraphWidgetOptions = {
*/
markings: "graph" | "grid" | "none";
// How to label the X and Y axis. default: ["x", "y"]
labels: ReadonlyArray<string>;
labels?: ReadonlyArray<string>;
// Whether to show the Protractor tool overlayed on top of the graph
showProtractor: boolean;
/**
Expand Down Expand Up @@ -1114,7 +1116,7 @@ export type PerseusNumericInputWidgetOptions = {
// A list of all the possible correct and incorrect answers
answers: ReadonlyArray<PerseusNumericInputAnswer>;
// Translatable Text; Text to describe this input. This will be shown to users using screenreaders.
labelText: string;
labelText?: string | undefined;
// Use size "Normal" for all text boxes, unless there are multiple text boxes in one line and the answer area is too narrow to fit them. Options: "normal" or "small"
size: string;
// A coefficient style number allows the student to use - for -1 and an empty string to mean 1.
Expand All @@ -1133,7 +1135,7 @@ export type PerseusNumericInputAnswer = {
// Translatable Display; A description for why this answer is correct, wrong, or ungraded
message: string;
// The expected answer
value: number;
value?: number;
// Whether this answer is "correct", "wrong", or "ungraded"
status: string;
// The forms available for this answer. Options: "integer, ""decimal", "proper", "improper", "mixed", or "pi"
Expand Down
22 changes: 22 additions & 0 deletions packages/perseus/src/util/parse-perseus-json/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -37,3 +37,25 @@ A good place to start reading this code is `parser-types.ts` and `result.ts`.
Then you should skim the parsers in `general-purpose-parsers/` to get a sense
of what's available. The Perseus-specific parsers are all in `perseus-parsers/`.
The public API is in `index.ts`.

### Default values

One architectural question that arose during the creation of these parsers was
"where should defaults be applied? In the parser, or in the widget's
`defaultProps`?"

Our answer, for the time being, is "both." These defaults serve different
purposes.

The `defaultProps` of a widget are used during content editing to initialize
the widget options. They represent our _current version_ of the initial widget
configuration that content creators should see when they add a widget to an
exercise. (Actually, the situation is slightly more complicated than this,
because widget editors can have default props as well.)

The defaults in the parsers represent the values that should be used for
_old content_, created before the associated prop was added to the widget.
**The default values in the parser should not change over time**, because any
changes might cause old exercises to become unsolvable or otherwise
misconfigured. The snapshot regression tests in this folder are designed to
ensure that we don't accidentally change the defaults.
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import {boolean, constant, object, string} from "../general-purpose-parsers";
import {defaulted} from "../general-purpose-parsers/defaulted";

import {parseWidget} from "./widget";

Expand All @@ -10,6 +11,6 @@ export const parseDefinitionWidget: Parser<DefinitionWidget> = parseWidget(
object({
togglePrompt: string,
definition: string,
static: boolean,
static: defaulted(boolean, () => false),
}),
);
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,13 @@ import {parseWidget} from "./widget";
import type {InputNumberWidget} from "../../../perseus-types";
import type {Parser} from "../parser-types";

const booleanToString: Parser<string> = (rawValue, ctx) => {
if (typeof rawValue === "boolean") {
return ctx.success(String(rawValue));
}
return ctx.failure("boolean", rawValue);
};

export const parseInputNumberWidget: Parser<InputNumberWidget> = parseWidget(
constant("input-number"),
object({
Expand All @@ -34,7 +41,11 @@ export const parseInputNumberWidget: Parser<InputNumberWidget> = parseWidget(
rightAlign: optional(boolean),
simplify: enumeration("required", "optional", "enforced"),
size: enumeration("normal", "small"),
value: union(number).or(string).parser,
// TODO(benchristel): there are some content items where value is a
// boolean, even though that makes no sense. We should figure out if
// those content items are actually published anywhere, and consider
// updating them.
value: union(number).or(string).or(booleanToString).parser,
customKeypad: optional(boolean),
}),
);
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import {
trio,
union,
} from "../general-purpose-parsers";
import {defaulted} from "../general-purpose-parsers/defaulted";

import {parsePerseusImageBackground} from "./perseus-image-background";
import {parseWidget} from "./widget";
Expand Down Expand Up @@ -276,18 +277,27 @@ export const parseInteractiveGraphWidget: Parser<InteractiveGraphWidget> =
constant("interactive-graph"),
object({
step: pairOfNumbers,
gridStep: pairOfNumbers,
snapStep: pairOfNumbers,
// TODO(benchristel): rather than making gridStep and snapStep
// optional, we should duplicate the defaulting logic from the
// InteractiveGraph component. See parse-perseus-json/README.md for
// why.
gridStep: optional(pairOfNumbers),
snapStep: optional(pairOfNumbers),
benchristel marked this conversation as resolved.
Show resolved Hide resolved
backgroundImage: optional(parsePerseusImageBackground),
markings: enumeration("graph", "grid", "none"),
labels: array(string),
labels: optional(array(string)),
showProtractor: boolean,
showRuler: optional(boolean),
showTooltips: optional(boolean),
rulerLabel: optional(string),
rulerTicks: optional(number),
range: pair(pairOfNumbers, pairOfNumbers),
graph: parsePerseusGraphType,
// NOTE(benchristel): I copied the default graph from
// interactive-graph.tsx. See the parse-perseus-json/README.md for
// an explanation of why we want to duplicate the default here.
graph: defaulted(parsePerseusGraphType, () => ({
type: "linear" as const,
})),
correct: parsePerseusGraphType,
// TODO(benchristel): default lockedFigures to empty array
lockedFigures: optional(array(parseLockedFigure)),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import {
optional,
nullable,
} from "../general-purpose-parsers";
import {defaulted} from "../general-purpose-parsers/defaulted";

import {parseWidget} from "./widget";

Expand All @@ -24,7 +25,10 @@ export const parseNumberLineWidget: Parser<NumberLineWidget> = parseWidget(
isTickCtrl: optional(nullable(boolean)),
divisionRange: array(number),
numDivisions: optional(nullable(number)),
snapDivisions: number,
// NOTE(benchristel): I copied the default snapDivisions from
// number-line.tsx. See the parse-perseus-json/README.md for
// an explanation of why we want to duplicate the default here.
snapDivisions: defaulted(number, () => 2),
tickStep: optional(nullable(number)),
correctRel: optional(nullable(string)),
correctX: number,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import {
boolean,
nullable,
} from "../general-purpose-parsers";
import {defaulted} from "../general-purpose-parsers/defaulted";

import {parseWidget} from "./widget";

Expand All @@ -31,19 +32,19 @@ export const parseNumericInputWidget: Parser<NumericInputWidget> = parseWidget(
answers: array(
object({
message: string,
value: number,
value: optional(number),
status: string,
answerForms: optional(array(parseMathFormat)),
strict: boolean,
maxError: optional(nullable(number)),
simplify: optional(nullable(string)),
}),
),
labelText: string,
labelText: optional(string),
size: string,
coefficient: boolean,
rightAlign: optional(boolean),
static: boolean,
static: defaulted(boolean, () => false),
answerForms: optional(
array(
object({
Expand Down
Original file line number Diff line number Diff line change
@@ -1,10 +1,17 @@
import {array, constant, enumeration, object} from "../general-purpose-parsers";
import {
array,
constant,
enumeration,
object,
pipeParsers,
} from "../general-purpose-parsers";
import {defaulted} from "../general-purpose-parsers/defaulted";

import {parsePerseusRenderer} from "./perseus-renderer";
import {parseWidget} from "./widget";

import type {OrdererWidget} from "../../../perseus-types";
import type {Parser} from "../parser-types";
import type {Parser, PartialParser} from "../parser-types";

// There is an import cycle between orderer-widget.ts and perseus-renderer.ts.
// This wrapper ensures that we don't refer to parsePerseusRenderer before
Expand All @@ -13,13 +20,28 @@ function parseRenderer(rawValue, ctx) {
return parsePerseusRenderer(rawValue, ctx);
}

const largeToAuto: PartialParser<
"normal" | "auto" | "large",
"normal" | "auto"
> = (height, ctx) => {
if (height === "large") {
return ctx.success("auto");
}
return ctx.success(height);
};

export const parseOrdererWidget: Parser<OrdererWidget> = parseWidget(
constant("orderer"),
object({
options: array(parseRenderer),
options: defaulted(array(parseRenderer), () => []),
correctOptions: array(parseRenderer),
otherOptions: array(parseRenderer),
height: enumeration("normal", "auto"),
layout: enumeration("horizontal", "vertical"),
height: pipeParsers(enumeration("normal", "auto", "large")).then(
largeToAuto,
).parser,
layout: defaulted(
enumeration("horizontal", "vertical"),
() => "horizontal" as const,
),
}),
);
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import {constant, object, string, boolean} from "../general-purpose-parsers";
import {defaulted} from "../general-purpose-parsers/defaulted";

import {parseWidget} from "./widget";

Expand All @@ -12,6 +13,6 @@ export const parsePassageWidget: Parser<PassageWidget> = parseWidget(
passageText: string,
passageTitle: string,
showLineNumbers: boolean,
static: boolean,
static: defaulted(boolean, () => false),
}),
);
Loading
Loading