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.

Sign up for GitHub

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 regression tests for PerseusItem parser #1907

Merged
merged 15 commits into from
Nov 26, 2024
Merged

Add and pass regression tests for PerseusItem parser #1907

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
64b514d
Pass regression test for matrix widget
benchristel Nov 22, 2024
367580b
Extract parseImages
benchristel Nov 22, 2024
647f501
Pass regression test for categorizer.options.static
benchristel Nov 22, 2024
3e2175b
Default radio choice content to empty string
benchristel Nov 22, 2024
3d27504
Pass another regression test
benchristel Nov 22, 2024
c5a0cf8
Default question content to empty string
benchristel Nov 22, 2024
67cf0d9
Pass another regression test
benchristel Nov 22, 2024
9df5170
Fix lint
benchristel Nov 22, 2024
8fb3143
Pass another regression test
benchristel Nov 22, 2024
29a9466
Update packages/perseus/src/widgets/graded-group/graded-group.tsx
benchristel Nov 25, 2024
8cadbcf
Update tests
benchristel Nov 26, 2024
b0ed10b
Fix formatting
benchristel Nov 26, 2024
d910fe6
Add changeset
benchristel Nov 26, 2024
adcea9c
Fix formatting
benchristel Nov 26, 2024
7aca4b6
Fix lint
benchristel Nov 26, 2024
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/chilled-turtles-drive.md
View file
Open in desktop
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.
19 changes: 12 additions & 7 deletions packages/perseus/src/perseus-types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,9 +93,14 @@ export type PerseusItem = {
hints: ReadonlyArray<Hint>;
// Details about the tools the user might need to answer the question
answerArea: PerseusAnswerArea | null | undefined;
// The version of the item. Not used by Perseus
itemDataVersion: Version;
// Deprecated field
/**
* The version of the item.
* @deprecated Not used.
*/
itemDataVersion: any;
/**
* @deprecated Superseded by per-widget answers.
*/
answer: any;
};

Expand Down Expand Up @@ -1034,17 +1039,17 @@ export type PerseusMatcherWidgetOptions = {
export type PerseusMatrixWidgetAnswers = ReadonlyArray<ReadonlyArray<number>>;
export type PerseusMatrixWidgetOptions = {
// Translatable Text; Shown before the matrix
prefix: string;
prefix?: string | undefined;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Doesn't the ? make this optional (ie. making 'undefined' a legal "value")?

Seeing both ? and undefined feels like we're saying the same thing twice.

Suggested change
prefix?: string | undefined;
prefix?: string;

Copy link
Member Author

Choose a reason for hiding this comment

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

You're right. I thought prefix?: string wouldn't allow the field to be explictly set to undefined, but that appears not to be the case.

// Translatable Text; Shown after the matrix
suffix: string;
suffix?: string | undefined;
// A data matrix representing the "correct" answers to be entered into the matrix
answers: PerseusMatrixWidgetAnswers;
// The coordinate location of the cursor position at start. default: [0, 0]
cursorPosition: ReadonlyArray<number>;
cursorPosition?: ReadonlyArray<number> | undefined;
// The coordinate size of the matrix. Only supports 2-dimensional matrix. default: [3, 3]
matrixBoardSize: ReadonlyArray<number>;
// Whether this is meant to statically display the answers (true) or be used as an input field, graded against the answers
static: boolean;
static?: boolean | undefined;
};

export type PerseusMeasurerWidgetOptions = {
Expand Down
2 changes: 1 addition & 1 deletion packages/perseus/src/util/parse-perseus-json/general-purpose-parsers/defaulted.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import {success} from "../result";

import type {Parser} from "../parser-types";

export function defaulted<T, Default extends T | null | undefined>(
export function defaulted<T, Default>(
parser: Parser<T>,
fallback: (missingValue: null | undefined) => Default,
): Parser<T | Default> {
Expand Down
6 changes: 4 additions & 2 deletions packages/perseus/src/util/parse-perseus-json/parse-perseus-json.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,11 +24,13 @@ describe("parseAndTypecheckPerseusItem", () => {
});

it("returns an error given an invalid PerseusItem", () => {
const result = parseAndTypecheckPerseusItem(`{ "bad": "value" }`);
const result = parseAndTypecheckPerseusItem(
`{"question": "bad value"}`,
);

assertFailure(result);
expect(result.detail).toContain(
"At (root).question -- expected object, but got undefined",
`At (root).question -- expected object, but got "bad value"`,
);
});
});
3 changes: 2 additions & 1 deletion packages/perseus/src/util/parse-perseus-json/perseus-parsers/categorizer-widget.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import {
optional,
string,
} from "../general-purpose-parsers";
import {defaulted} from "../general-purpose-parsers/defaulted";

import {parseWidget} from "./widget";

Expand All @@ -19,7 +20,7 @@ export const parseCategorizerWidget: Parser<CategorizerWidget> = parseWidget(
items: array(string),
categories: array(string),
randomizeItems: boolean,
static: boolean,
static: defaulted(boolean, () => false),
values: array(number),
highlightLint: optional(boolean),
linterContext: optional(
Copy link
Collaborator

Choose a reason for hiding this comment

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

This definitely shouldn't be part of the serialized widget options for categorizer. It is a prop passed down by the editor components during content authoring. I can imagine this is in our published content because of a bug somewhere.

No action required, just musing.

Expand Down
3 changes: 2 additions & 1 deletion packages/perseus/src/util/parse-perseus-json/perseus-parsers/dropdown-widget.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import {
object,
string,
} from "../general-purpose-parsers";
import {defaulted} from "../general-purpose-parsers/defaulted";

import {parseWidget} from "./widget";

Expand All @@ -15,7 +16,7 @@ export const parseDropdownWidget: Parser<DropdownWidget> = parseWidget(
constant("dropdown"),
object({
placeholder: string,
static: boolean,
static: defaulted(boolean, () => false),
choices: array(
object({
content: string,
Expand Down
7 changes: 6 additions & 1 deletion packages/perseus/src/util/parse-perseus-json/perseus-parsers/expression-widget.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,12 @@ import {
boolean,
constant,
enumeration,
number,
object,
optional,
pipeParsers,
string,
union,
} from "../general-purpose-parsers";

import {parseWidget} from "./widget";
Expand All @@ -21,7 +24,9 @@ const parseAnswerForm: Parser<PerseusExpressionAnswerForm> = object({
form: boolean,
simplify: boolean,
considered: enumeration("correct", "wrong", "ungraded"),
key: optional(string),
key: pipeParsers(optional(union(string).or(number).parser)).then(
(key, ctx) => ctx.success(String(key)),
).parser,
});

export const parseExpressionWidget: Parser<ExpressionWidget> = parseWidget(
Expand Down
3 changes: 2 additions & 1 deletion packages/perseus/src/util/parse-perseus-json/perseus-parsers/graded-group-widget.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import {
record,
string,
} from "../general-purpose-parsers";
import {defaulted} from "../general-purpose-parsers/defaulted";

import {parsePerseusRenderer} from "./perseus-renderer";
import {parseWidget} from "./widget";
Expand All @@ -17,7 +18,7 @@ import type {GradedGroupWidget} from "../../../perseus-types";
import type {Parser} from "../parser-types";

export const parseGradedGroupWidgetOptions = object({
title: string,
title: defaulted(string, () => ""),
hasHint: optional(nullable(boolean)),
// This module has an import cycle with parsePerseusRenderer.
// The anonymous function below ensures that we don't try to access
Expand Down
14 changes: 4 additions & 10 deletions packages/perseus/src/util/parse-perseus-json/perseus-parsers/hint.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,13 @@
import {
array,
boolean,
number,
object,
optional,
record,
string,
} from "../general-purpose-parsers";
import {defaulted} from "../general-purpose-parsers/defaulted";

import {parseImages} from "./images-map";
import {parseWidgetsMap} from "./widgets-map";

import type {Hint} from "../../../perseus-types";
Expand All @@ -16,13 +16,7 @@ import type {Parser} from "../parser-types";
export const parseHint: Parser<Hint> = object({
replace: optional(boolean),
content: string,
widgets: parseWidgetsMap,
widgets: defaulted(parseWidgetsMap, () => ({})),
metadata: optional(array(string)),
images: record(
string,
object({
width: number,
height: number,
}),
),
images: parseImages,
});
3 changes: 2 additions & 1 deletion packages/perseus/src/util/parse-perseus-json/perseus-parsers/iframe-widget.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import {
string,
union,
} from "../general-purpose-parsers";
import {defaulted} from "../general-purpose-parsers/defaulted";

import {parseWidget} from "./widget";

Expand All @@ -23,6 +24,6 @@ export const parseIframeWidget: Parser<IFrameWidget> = parseWidget(
height: union(number).or(string).parser,
allowFullScreen: boolean,
allowTopNavigation: optional(boolean),
static: boolean,
static: defaulted(boolean, () => false),
}),
);
17 changes: 17 additions & 0 deletions packages/perseus/src/util/parse-perseus-json/perseus-parsers/images-map.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
import {number, object, record, string} from "../general-purpose-parsers";
import {defaulted} from "../general-purpose-parsers/defaulted";

import type {PerseusImageDetail} from "../../../perseus-types";
import type {Parser} from "../parser-types";

export const parseImages: Parser<{[key: string]: PerseusImageDetail}> =
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not a required change, but the keys in these image maps should be the url to the image. Clarifying the label may be helpful for others.

Suggested change
export const parseImages: Parser<{[key: string]: PerseusImageDetail}> =
export const parseImages: Parser<{[url: string]: PerseusImageDetail}> =

defaulted(
record(
string,
object({
width: number,
height: number,
}),
),
() => ({}),
);
12 changes: 7 additions & 5 deletions packages/perseus/src/util/parse-perseus-json/perseus-parsers/matrix-widget.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,22 +4,24 @@ import {
constant,
number,
object,
optional,
string,
} from "../general-purpose-parsers";
import {defaulted} from "../general-purpose-parsers/defaulted";

import {parseWidget} from "./widget";

import type {MatrixWidget} from "../../../perseus-types";
import type {Parser} from "../parser-types";

export const parseMatrixWidget: Parser<MatrixWidget> = parseWidget(
constant("matrix"),
defaulted(constant("matrix"), () => "matrix"),
object({
prefix: string,
suffix: string,
prefix: optional(string),
suffix: optional(string),
answers: array(array(number)),
cursorPosition: array(number),
cursorPosition: optional(array(number)),
matrixBoardSize: array(number),
static: boolean,
static: optional(boolean),
}),
);
16 changes: 10 additions & 6 deletions packages/perseus/src/util/parse-perseus-json/perseus-parsers/perseus-item.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,11 @@ import {
enumeration,
number,
object,
optional,
pipeParsers,
record,
} from "../general-purpose-parsers";
import {defaulted} from "../general-purpose-parsers/defaulted";

import {parseHint} from "./hint";
import {parsePerseusRenderer} from "./perseus-renderer";
Expand All @@ -18,14 +20,16 @@ import type {ParseContext, Parser, ParseResult} from "../parser-types";

export const parsePerseusItem: Parser<PerseusItem> = object({
question: parsePerseusRenderer,
hints: array(parseHint),
answerArea: pipeParsers(object({}))
hints: defaulted(array(parseHint), () => []),
answerArea: pipeParsers(defaulted(object({}), () => ({})))
.then(migrateAnswerArea)
.then(record(enumeration(...ItemExtras), boolean)).parser,
itemDataVersion: object({
major: number,
minor: number,
}),
itemDataVersion: optional(
object({
major: number,
minor: number,
}),
),
// Deprecated field
answer: any,
});
Expand Down
53 changes: 24 additions & 29 deletions packages/perseus/src/util/parse-perseus-json/perseus-parsers/perseus-renderer.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,37 +1,32 @@
import {
array,
number,
object,
optional,
record,
string,
} from "../general-purpose-parsers";
import {array, object, optional, string} from "../general-purpose-parsers";
import {defaulted} from "../general-purpose-parsers/defaulted";

import {parseImages} from "./images-map";
import {parseWidgetsMap} from "./widgets-map";

import type {PerseusRenderer} from "../../../perseus-types";
import type {Parser} from "../parser-types";

export const parsePerseusRenderer: Parser<PerseusRenderer> = object({
content: string,
// This module has an import cycle with parseWidgetsMap, because the
// `group` widget can contain another renderer.
// The anonymous function below ensures that we don't try to access
// parseWidgetsMap before it's defined.
widgets: defaulted(
(rawVal, ctx) => parseWidgetsMap(rawVal, ctx),
() => ({}),
),
metadata: optional(array(string)),
images: defaulted(
record(
string,
object({
width: number,
height: number,
}),
export const parsePerseusRenderer: Parser<PerseusRenderer> = defaulted(
object({
// TODO(benchristel): content is also defaulted to empty string in
// renderer.tsx. See if we can remove one default or the other.
Comment on lines +12 to +13
Copy link
Collaborator

Choose a reason for hiding this comment

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

I've been thinking about this as I work through this PR. Some of our Perseus widgets/components provide default props. There are places we might be able to de-duplicate these defaults to only be defaulted in our parsers. But there may be some complications on the editing side as I'm pretty sure they depend on those default props.

Copy link
Member Author

@benchristel benchristel Nov 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I've been thinking about this too. I believe the correct place to add default values is in the parsing layer. That ensures that old content always renders consistently even if we change the default props used by the widget editors. We may still need defaultProps in the widgets too, and I think that's okay.

For example, imagine we changed the default snapStep for InteractiveGraph from 0.5 to 1 in the widget's default props. If we relied on those default props to migrate legacy data that was missing snapStep, the change would propagate to a lot of existing content — some of which might not be answerable with the new default snapStep.

If, on the other hand, we defaulted snapStep to 0.5 in the parsing layer, old content would be insulated from the change to the widget editor.

By defaulting values in the parser, we can decouple the "defaults for legacy content" from the "defaults for content creators".

content: defaulted(string, () => ""),
// This module has an import cycle with parseWidgetsMap, because the
// `group` widget can contain another renderer.
// The anonymous function below ensures that we don't try to access
// parseWidgetsMap before it's defined.
widgets: defaulted(
(rawVal, ctx) => parseWidgetsMap(rawVal, ctx),
() => ({}),
),
() => ({}),
),
});
metadata: optional(array(string)),
images: parseImages,
}),
// Default value
() => ({
content: "",
widgets: {},
images: {},
}),
);
3 changes: 2 additions & 1 deletion packages/perseus/src/util/parse-perseus-json/perseus-parsers/radio-widget.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import {
optional,
string,
} from "../general-purpose-parsers";
import {defaulted} from "../general-purpose-parsers/defaulted";

import {parseWidget} from "./widget";
import {parseWidgetsMap} from "./widgets-map";
Expand All @@ -19,7 +20,7 @@ export const parseRadioWidget: Parser<RadioWidget> = parseWidget(
object({
choices: array(
object({
content: string,
content: defaulted(string, () => ""),
Copy link
Collaborator

Choose a reason for hiding this comment

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

I wonder if defaulted() would be easier to use if were defined such that the second parameter could be the literal default value as an option to a function that built the default value.

Something like this:

defaulted<T|Defaulted>(Parser<T>, Defaulted | () => Defaulted)

That'd allow many of these usages to be simpler, like this:

Suggested change
content: defaulted(string, () => ""),
content: defaulted(string, ""),

Copy link
Member Author

Choose a reason for hiding this comment

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

The danger with that approach is that if the default value is an object or array, the same instance will be reused for every default. E.g.

defaulted(array(someParser), [])

If some other code later mutates the array, it might affect unrelated code that happens to be holding a reference to the same value. This kind of "aliasing" bug has bitten me many times in my career, and it's always a nightmare.

I believe we have a pretty good culture of avoiding unnecessary mutation, but aliasing bugs are so bad that I feel it's better to be safe than sorry.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Fair point. Let's leave it as a function then.

clue: optional(string),
correct: optional(boolean),
isNoneOfTheAbove: optional(boolean),
Expand Down
10 changes: 6 additions & 4 deletions packages/perseus/src/util/parse-perseus-json/perseus-parsers/widget.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,9 +20,11 @@ export function parseWidget<Type extends string, Options>(
alignment: optional(string),
options: parseOptions,
key: optional(number),
version: object({
major: number,
minor: number,
}),
version: optional(
object({
major: number,
minor: number,
Comment on lines +25 to +26
Copy link
Collaborator

Choose a reason for hiding this comment

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

Related to other notes about defaults at different parts of Perseus, I wonder if we should make these defaulted() to 0 also? Ultimately, I suspect that the entire concept of versioning may go away because we'll just have "evergreen" parsers that handle all versions of content we've ever seen (and haven't backfilled). 🤔

No action needed, just an idea.

https://github.com/Khan/perseus/blob/1e5fce35d12efdb7bcc17f0c841da95957f9a20e/packages/perseus/src/widgets.ts#L226

Copy link
Member Author

Choose a reason for hiding this comment

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

Those defaults would let us handle data like version: {}, right? I haven't seen any data like that. If we ever encountered it in the wild, I'd want to get a parse error rather than just silently converting it to {major: 0, minor: 0}, because it would indicate that we have a bug somewhere else.

Evergreen parsers are a possibility, but I think we might sometimes want versioning to help make complex migrations typesafe and predictable. I do wish version were just a number; that would be a lot easier to deal with in TypeScript.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Not for version: {} but for items that have no version key whatsoever.

oldWidgetInfo.version || {major: 0, minor: 0}; would only use the value after || if it is falsy, I believe.

}),
),
});
}
Loading