Add and pass more regression tests for PerseusItem parser (#1952)

Issue: LEMS-2582

## Test plan:

`yarn test`

Author: benchristel

Reviewers: jeremywiebe, benchristel

Required Reviewers:

Approved By: jeremywiebe

Checks: ✅ Lint, Typecheck, Format, and Test (ubuntu-latest, 20.x), ✅ Publish npm snapshot (ubuntu-latest, 20.x), ✅ Cypress (ubuntu-latest, 20.x), ✅ Check builds for changes in size (ubuntu-latest, 20.x), ✅ Check for .changeset entries for all changed files (ubuntu-latest, 20.x), ✅ Publish Storybook to Chromatic (ubuntu-latest, 20.x)

Pull Request URL: #1952

.changeset/sixty-vans-end.md

@@ -0,0 +1,5 @@
+---
+"@khanacademy/perseus": patch
+---
+
+Internal: add and pass more regression tests for PerseusItem parser

packages/perseus/src/perseus-types.ts

@@ -116,6 +116,7 @@ export interface PerseusWidgetTypes {
     video: VideoWidget;
 
     // Deprecated widgets
+    "lights-puzzle": AutoCorrectWidget;
     sequence: AutoCorrectWidget;
 }
 
@@ -1176,7 +1177,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 | null;
     // Whether this answer is "correct", "wrong", or "ungraded"
     status: string;
     // The forms available for this answer.  Options: "integer, ""decimal", "proper", "improper", "mixed", or "pi"
@@ -1255,7 +1256,7 @@ export type PerseusPassageRefWidgetOptions = {
     // The reference number
     referenceNumber: number;
     // Short summary of the referenced section. This will be included in parentheses and quotes automatically.
-    summaryText: string;
+    summaryText?: string;
 };
 
 export const plotterPlotTypes = [

packages/perseus/src/util/parse-perseus-json/general-purpose-parsers/convert.ts

@@ -0,0 +1,7 @@
+import type {PartialParser} from "../parser-types";
+
+// Given a function, creates a PartialParser that converts one type to another
+// using that function. The returned parser never fails.
+export function convert<A, B>(f: (value: A) => B): PartialParser<A, B> {
+    return (rawValue, ctx) => ctx.success(f(rawValue));
+}

packages/perseus/src/util/parse-perseus-json/general-purpose-parsers/discriminated-union.test.ts

@@ -0,0 +1,84 @@
+import {parse} from "../parse";
+import {failure, success} from "../result";
+
+import {constant} from "./constant";
+import {discriminatedUnion} from "./discriminated-union";
+import {number} from "./number";
+import {object} from "./object";
+
+describe("a discriminatedUnion with one variant", () => {
+    const unionParser = discriminatedUnion(
+        object({type: constant("ok")}),
+        object({type: constant("ok"), value: number}),
+    ).parser;
+
+    it("parses a valid value", () => {
+        const input = {type: "ok", value: 3};
+
+        expect(parse(input, unionParser)).toEqual(success(input));
+    });
+
+    it("rejects a value with the wrong `type`", () => {
+        const input = {type: "bad", value: 3};
+
+        expect(parse(input, unionParser)).toEqual(
+            failure(`At (root).type -- expected "ok", but got "bad"`),
+        );
+    });
+
+    it("rejects a value with a valid type but wrong fields", () => {
+        const input = {type: "ok", value: "foobar"};
+
+        expect(parse(input, unionParser)).toEqual(
+            failure(`At (root).value -- expected number, but got "foobar"`),
+        );
+    });
+});
+
+describe("a discriminatedUnion with two variants", () => {
+    const unionParser = discriminatedUnion(
+        object({type: constant("rectangle")}),
+        object({type: constant("rectangle"), width: number}),
+    ).or(
+        object({type: constant("circle")}),
+        object({type: constant("circle"), radius: number}),
+    ).parser;
+
+    it("parses a valid rectangle", () => {
+        const input = {type: "rectangle", width: 42};
+
+        expect(parse(input, unionParser)).toEqual(success(input));
+    });
+
+    it("rejects a rectangle with no width", () => {
+        const input = {type: "rectangle", radius: 99};
+
+        expect(parse(input, unionParser)).toEqual(
+            failure(`At (root).width -- expected number, but got undefined`),
+        );
+    });
+
+    it("parses a valid circle", () => {
+        const input = {type: "circle", radius: 7};
+
+        expect(parse(input, unionParser)).toEqual(success(input));
+    });
+
+    it("rejects a circle with no radius", () => {
+        const input = {type: "circle", width: 99};
+
+        expect(parse(input, unionParser)).toEqual(
+            failure(`At (root).radius -- expected number, but got undefined`),
+        );
+    });
+
+    it("rejects a value with an unrecognized `type`", () => {
+        const input = {type: "triangle", width: -1, radius: 99};
+
+        expect(parse(input, unionParser)).toEqual(
+            failure(
+                `At (root).type -- expected "rectangle", but got "triangle"`,
+            ),
+        );
+    });
+});

packages/perseus/src/util/parse-perseus-json/general-purpose-parsers/discriminated-union.ts

@@ -0,0 +1,46 @@
+import {isSuccess} from "../result";
+
+import {pipeParsers} from "./pipe-parsers";
+
+import type {Parser} from "../parser-types";
+
+// discriminatedUnion() should be preferred over union() when parsing a
+// discriminated union type, because discriminatedUnion() produces more
+// understandable failure messages. It takes the discriminant as the source of
+// truth for which variant is to be parsed, and expects the other data to match
+// that variant.
+export function discriminatedUnion<T>(
+    narrow: Parser<unknown>,
+    parseVariant: Parser<T>,
+): DiscriminatedUnionBuilder<T> {
+    return new DiscriminatedUnionBuilder(
+        pipeParsers(narrow).then(parseVariant).parser,
+    );
+}
+
+class DiscriminatedUnionBuilder<Variant> {
+    constructor(public parser: Parser<Variant>) {}
+
+    or<Variant2>(
+        narrow: Parser<unknown>,
+        parseVariant: Parser<Variant2>,
+    ): DiscriminatedUnionBuilder<Variant | Variant2> {
+        return new DiscriminatedUnionBuilder(
+            either(narrow, parseVariant, this.parser),
+        );
+    }
+}
+
+function either<A, B>(
+    narrowToA: Parser<unknown>,
+    parseA: Parser<A>,
+    parseB: Parser<B>,
+): Parser<A | B> {
+    return (rawValue, ctx) => {
+        if (isSuccess(narrowToA(rawValue, ctx))) {
+            return parseA(rawValue, ctx);
+        }
+
+        return parseB(rawValue, ctx);
+    };
+}

packages/perseus/src/util/parse-perseus-json/perseus-parsers/explanation-widget.ts

@@ -1,4 +1,5 @@
 import {boolean, constant, object, string} from "../general-purpose-parsers";
+import {defaulted} from "../general-purpose-parsers/defaulted";
 
 import {parseWidget} from "./widget";
 import {parseWidgetsMap} from "./widgets-map";
@@ -15,7 +16,10 @@ export const parseExplanationWidget: Parser<ExplanationWidget> = parseWidget(
         // We wrap parseWidgetsMap in a function here to make sure it is not
         // referenced before it is defined. There is an import cycle between
         // this file and widgets-map.ts that could cause it to be undefined.
-        widgets: (rawVal, ctx) => parseWidgetsMap(rawVal, ctx),
-        static: boolean,
+        widgets: defaulted(
+            (rawVal, ctx) => parseWidgetsMap(rawVal, ctx),
+            () => ({}),
+        ),
+        static: defaulted(boolean, () => false),
     }),
 );

packages/perseus/src/util/parse-perseus-json/perseus-parsers/expression-widget.ts

@@ -10,36 +10,54 @@ import {
     string,
     union,
 } from "../general-purpose-parsers";
+import {convert} from "../general-purpose-parsers/convert";
+import {defaulted} from "../general-purpose-parsers/defaulted";
 
+import {versionedWidgetOptions} from "./versioned-widget-options";
 import {parseWidgetWithVersion} from "./widget";
 
 import type {
     ExpressionWidget,
     PerseusExpressionAnswerForm,
 } from "../../../perseus-types";
-import type {
-    ParseContext,
-    ParsedValue,
-    Parser,
-    ParseResult,
-} from "../parser-types";
+import type {ParsedValue, Parser} from "../parser-types";
 
-const parseAnswerForm: Parser<PerseusExpressionAnswerForm> = object({
-    value: string,
-    form: boolean,
-    simplify: boolean,
+const parsePossiblyInvalidAnswerForm = object({
+    // `value` is the possibly invalid part of this. It should always be a
+    // string, but some answer forms don't have it. The Expression widget
+    // ignores invalid values, so we can safely filter them out during parsing.
+    value: optional(string),
+    form: defaulted(boolean, () => false),
+    simplify: defaulted(boolean, () => false),
     considered: enumeration("correct", "wrong", "ungraded"),
     key: pipeParsers(optional(union(string).or(number).parser)).then(
         (key, ctx) => ctx.success(String(key)),
     ).parser,
 });
 
+function removeInvalidAnswerForms(
+    possiblyInvalid: Array<ParsedValue<typeof parsePossiblyInvalidAnswerForm>>,
+): PerseusExpressionAnswerForm[] {
+    const valid: PerseusExpressionAnswerForm[] = [];
+    for (const answerForm of possiblyInvalid) {
+        const {value} = answerForm;
+        if (value != null) {
+            // Copying the object seems to be needed to make TypeScript happy
+            valid.push({...answerForm, value});
+        }
+    }
+    return valid;
+}
+
+const version1 = object({major: constant(1), minor: number});
 const parseExpressionWidgetV1: Parser<ExpressionWidget> =
     parseWidgetWithVersion(
-        object({major: constant(1), minor: number}),
+        version1,
         constant("expression"),
         object({
-            answerForms: array(parseAnswerForm),
+            answerForms: pipeParsers(
+                array(parsePossiblyInvalidAnswerForm),
+            ).then(convert(removeInvalidAnswerForms)).parser,
             functions: array(string),
             times: boolean,
             visibleLabel: optional(string),
@@ -59,8 +77,9 @@ const parseExpressionWidgetV1: Parser<ExpressionWidget> =
         }),
     );
 
+const version0 = optional(object({major: constant(0), minor: number}));
 const parseExpressionWidgetV0 = parseWidgetWithVersion(
-    optional(object({major: constant(0), minor: number})),
+    version0,
     constant("expression"),
     object({
         functions: array(string),
@@ -87,10 +106,9 @@ const parseExpressionWidgetV0 = parseWidgetWithVersion(
 
 function migrateV0ToV1(
     widget: ParsedValue<typeof parseExpressionWidgetV0>,
-    ctx: ParseContext,
-): ParseResult<ExpressionWidget> {
+): ExpressionWidget {
     const {options} = widget;
-    return ctx.success({
+    return {
         ...widget,
         version: {major: 1, minor: 0},
         options: {
@@ -110,9 +128,11 @@ function migrateV0ToV1(
                 },
             ],
         },
-    });
+    };
 }
 
-export const parseExpressionWidget: Parser<ExpressionWidget> = union(
-    parseExpressionWidgetV1,
-).or(pipeParsers(parseExpressionWidgetV0).then(migrateV0ToV1).parser).parser;
+export const parseExpressionWidget: Parser<ExpressionWidget> =
+    versionedWidgetOptions(parseExpressionWidgetV1).withMigrationFrom(
+        parseExpressionWidgetV0,
+        migrateV0ToV1,
+    ).parser;

packages/perseus/src/util/parse-perseus-json/perseus-parsers/grapher-widget.ts

@@ -11,6 +11,7 @@ import {
     string,
     union,
 } from "../general-purpose-parsers";
+import {discriminatedUnion} from "../general-purpose-parsers/discriminated-union";
 
 import {parseWidget} from "./widget";
 
@@ -35,45 +36,52 @@ export const parseGrapherWidget: Parser<GrapherWidget> = parseWidget(
                 "tangent",
             ),
         ),
-        correct: union(
+        correct: discriminatedUnion(
+            object({type: constant("absolute_value")}),
             object({
                 type: constant("absolute_value"),
                 coords: pairOfPoints,
             }),
         )
             .or(
+                object({type: constant("exponential")}),
                 object({
                     type: constant("exponential"),
                     asymptote: pairOfPoints,
                     coords: pairOfPoints,
                 }),
             )
             .or(
+                object({type: constant("linear")}),
                 object({
                     type: constant("linear"),
                     coords: pairOfPoints,
                 }),
             )
             .or(
+                object({type: constant("logarithm")}),
                 object({
                     type: constant("logarithm"),
                     asymptote: pairOfPoints,
                     coords: pairOfPoints,
                 }),
             )
             .or(
+                object({type: constant("quadratic")}),
                 object({
                     type: constant("quadratic"),
                     coords: pairOfPoints,
                 }),
             )
             .or(
+                object({type: constant("sinusoid")}),
                 object({
                     type: constant("sinusoid"),
                     coords: pairOfPoints,
                 }),
             )
             .or(
+                object({type: constant("tangent")}),
                 object({
                     type: constant("tangent"),
                     coords: pairOfPoints,