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 tests for propUpgrades functions #1914

Merged
merged 7 commits into from
Nov 27, 2024
Merged

Add tests for propUpgrades functions #1914

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
774f407
expression
handeyeco Nov 26, 2024
65c06c1
measurer
handeyeco Nov 26, 2024
250f67e
radio
handeyeco Nov 26, 2024
b2859e9
changeset
handeyeco Nov 26, 2024
febaac0
cleanup
handeyeco Nov 26, 2024
bbcfde0
respond to Jeremys feedback
handeyeco Nov 26, 2024
c12b3f6
add more types
handeyeco 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/two-feet-care.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@khanacademy/perseus": patch
---

Add tests for propUpgrades functions (and remove underscore usage)
29 changes: 29 additions & 0 deletions packages/perseus/src/widgets/expression/expression.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -523,4 +523,33 @@ describe("Expression Widget", function () {
).toBeNull();
});
});

describe("propUpgrades", () => {
it("can upgrade from v0 to v1", () => {
const v0props = {
times: false,
buttonSets: ["basic"],
functions: [],
form: false,
simplify: false,
value: "42",
};

const result = ExpressionWidgetExport.propUpgrades["1"](v0props);
Copy link
Collaborator

Choose a reason for hiding this comment

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

suggestion: Typically, I'm against explicit typing for variables, but in this case I think it's a good additional validation to type this result variable as PerseusExpressionWidgetOptions so that TypeScript ensures that the prop upgrade it typed correctly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I went back and forth on this. This won't work:

const result: WhateverWidgetOptions = WhateverWidgetExport.propUpgrades["1"](v0props);

...because the propUpgrades function takes any and modifies it (which basically makes the return value any). Also because 2/3 propUpgrades returned any (I just changed that).

So I did a mix:

const expected: WhateverWidgetOptions = {}
const result: WhateverWidgetOptions = WhateverWidgetExport.propUpgrades["1"](v0props);
expect(result).toEqual(expected)

Not sure how helpful it'll be though.


expect(result).toEqual({
times: false,
buttonSets: ["basic"],
functions: [],
answerForms: [
{
considered: "correct",
form: false,
simplify: false,
value: "42",
},
],
});
});
});
});
39 changes: 39 additions & 0 deletions packages/perseus/src/widgets/measurer/measurer.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
import MeasurerWidgetExport from "./measurer";

describe("measurer", () => {
describe("propUpgrades", () => {
it("can upgrade from v0 to v1", () => {
const v0props = {
imageUrl: "url",
imageTop: 42,
imageLeft: 42,
showProtractor: false,
showRuler: false,
rulerLabel: "test",
rulerTicks: 4,
rulerPixels: 4,
rulerLength: 4,
box: [4, 4],
static: false,
};

const result = MeasurerWidgetExport.propUpgrades["1"](v0props);
handeyeco marked this conversation as resolved.
Show resolved Hide resolved

expect(result).toEqual({
image: {
url: "url",
top: 42,
left: 42,
},
showProtractor: false,
showRuler: false,
rulerLabel: "test",
rulerTicks: 4,
rulerPixels: 4,
rulerLength: 4,
box: [4, 4],
static: false,
});
});
});
});
22 changes: 10 additions & 12 deletions packages/perseus/src/widgets/measurer/measurer.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -183,18 +183,16 @@ class Measurer extends React.Component<Props> implements Widget {

const propUpgrades = {
"1": (v0props: any): any => {
const v1props = _(v0props)
.chain()
.omit("imageUrl", "imageTop", "imageLeft")
.extend({
image: {
url: v0props.imageUrl,
top: v0props.imageTop,
left: v0props.imageLeft,
},
})
.value();
return v1props;
const {imageUrl, imageTop, imageLeft, ...rest} = v0props;

return {
...rest,
image: {
url: imageUrl,
top: imageTop,
left: imageLeft,
},
};
Comment on lines +186 to +195
Copy link
Collaborator

Choose a reason for hiding this comment

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

So much simpler and clearer than the underscore variant!

},
} as const;

Expand Down
27 changes: 27 additions & 0 deletions packages/perseus/src/widgets/radio/__tests__/radio.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import * as Dependencies from "../../../dependencies";
import {mockStrings} from "../../../strings";
import {renderQuestion} from "../../__testutils__/renderQuestion";
import PassageWidget from "../../passage";
import RadioWidgetExport from "../radio";
import scoreRadio from "../score-radio";

import {
Expand Down Expand Up @@ -984,3 +985,29 @@ describe("scoring", () => {
expect(renderer).toHaveBeenAnsweredIncorrectly();
});
});

describe("propsUpgrade", () => {
it("can upgrade from v0 to v1", () => {
const v0props = {
choices: [{content: "Choice 1"}, {content: "Choice 2"}],
};

const result = RadioWidgetExport.propUpgrades["1"](v0props);

expect(result).toEqual({
choices: [{content: "Choice 1"}, {content: "Choice 2"}],
hasNoneOfTheAbove: false,
});
});

it("throws from noneOfTheAbove", () => {
const v0props = {
choices: [{content: "Choice 1"}, {content: "Choice 2"}],
noneOfTheAbove: true,
};

expect(() => RadioWidgetExport.propUpgrades["1"](v0props)).toThrow(
"radio widget v0 no longer supports auto noneOfTheAbove",
);
});
});
16 changes: 6 additions & 10 deletions packages/perseus/src/widgets/radio/radio.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,22 +127,18 @@ const transform = (

const propUpgrades = {
"1": (v0props: any): any => {
let choices;
let hasNoneOfTheAbove;
const {noneOfTheAbove, ...rest} = v0props;

if (!v0props.noneOfTheAbove) {
choices = v0props.choices;
hasNoneOfTheAbove = false;
} else {
if (noneOfTheAbove) {
throw new Error(
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 what happens if this throws in prod. 🤔 🤔 🤔

No action required!

"radio widget v0 no longer supports auto noneOfTheAbove",
);
}

return _.extend(_.omit(v0props, "noneOfTheAbove"), {
choices: choices,
hasNoneOfTheAbove: hasNoneOfTheAbove,
});
return {
...rest,
hasNoneOfTheAbove: false,
};
},
} as const;

Expand Down