Split out validation logic for input-number #1640

Myranae · 2024-09-17T21:07:54Z

Summary:

For the server-side scoring project, we are moving validation logic out of each widget component. This moves the validation logic for the input-number widget.

Issue: LEMS-2355

Test plan:

All tests should pass
Nothing should change as code is just being moved into separate files

github-actions · 2024-09-17T21:12:14Z

npm Snapshot: Published

Good news!! We've packaged up the latest commit from this PR (d7da108) and published it to npm. You
can install it using the tag PR1640.

Example:

yarn add @khanacademy/perseus@PR1640

If you are working in Khan Academy's webapp, you can run:

./dev/tools/bump_perseus_version.sh -t PR1640

github-actions · 2024-09-17T21:12:32Z

Size Change: +6 B (0%)

Total Size: 861 kB

Filename	Size	Change
`packages/perseus/dist/es/index.js`	417 kB	+6 B (0%)

ℹ️ View Unchanged

Filename	Size
`packages/kas/dist/es/index.js`	38.3 kB
`packages/keypad-context/dist/es/index.js`	760 B
`packages/kmath/dist/es/index.js`	4.27 kB
`packages/math-input/dist/es/index.js`	78 kB
`packages/math-input/dist/es/strings.js`	1.79 kB
`packages/perseus-core/dist/es/index.js`	1.48 kB
`packages/perseus-editor/dist/es/index.js`	278 kB
`packages/perseus-linter/dist/es/index.js`	22.2 kB
`packages/perseus/dist/es/strings.js`	3.36 kB
`packages/pure-markdown/dist/es/index.js`	3.66 kB
`packages/simple-markdown/dist/es/index.js`	12.4 kB

_{compressed-size-action}

codecov · 2024-09-17T21:16:01Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 70.36%. Comparing base (64bcde0) to head (d7da108).
Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1640      +/-   ##
==========================================
- Coverage   70.60%   70.36%   -0.24%     
==========================================
  Files         557      585      +28     
  Lines      107581   112118    +4537     
  Branches     5257    11238    +5981     
==========================================
+ Hits        75955    78893    +2938     
- Misses      31507    33225    +1718     
+ Partials      119        0     -119

see 1142 files with indirect coverage changes

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 64bcde0...d7da108. Read the comment docs.

Myranae · 2024-09-17T21:18:34Z

packages/perseus/src/widgets/input-number/input-number-validator.test.ts

+    it("transform should remove the `value` field", function () {
+        const editorProps = {
+            value: 5,
+            simplify: "required",
+            size: "normal",
+            inexact: false,
+            maxError: 0.1,
+            answerType: "number",
+        } as const;
+        if (!transform) {
+            throw new Error("transform not defined");
+        }
+        const widgetProps = transform(editorProps, mockStrings);
+        expect(_.has(widgetProps, "value")).toBe(false);
+    });


This isn't related to scoring, but I didn't want to have all of the item info in two separate places. Feel free to let me know if that is preferred to having this test here though. Thanks!

The validator and associated tests probably need to be as decoupled from the component as possible, with no React stuff at all. So no rendering and nothing from React Testing Library, just tests for a pure function.

I'd put these React-y tests back where they were and add new validator-specific tests that pass in input/rubric and receive a score.

khan-actions-bot · 2024-09-17T21:19:11Z

Gerald

Required Reviewers

@Khan/perseus for changes to .changeset/modern-dancers-decide.md, packages/perseus/src/widgets/input-number/input-number-validator.test.ts, packages/perseus/src/widgets/input-number/input-number-validator.ts, packages/perseus/src/widgets/input-number/input-number.test.ts, packages/perseus/src/widgets/input-number/input-number.tsx, packages/perseus/src/widgets/input-number/input-number.types.ts

Don't want to be involved in this pull request? Comment #removeme and we won't notify you of further changes.

handeyeco

Just need to further decouple validator logic from React logic

handeyeco · 2024-09-18T15:44:38Z

packages/perseus/src/widgets/input-number/input-number-validator.test.ts

+    it("transform should remove the `value` field", function () {
+        const editorProps = {
+            value: 5,
+            simplify: "required",
+            size: "normal",
+            inexact: false,
+            maxError: 0.1,
+            answerType: "number",
+        } as const;
+        if (!transform) {
+            throw new Error("transform not defined");
+        }
+        const widgetProps = transform(editorProps, mockStrings);
+        expect(_.has(widgetProps, "value")).toBe(false);
+    });


The validator and associated tests probably need to be as decoupled from the component as possible, with no React stuff at all. So no rendering and nothing from React Testing Library, just tests for a pure function.

I'd put these React-y tests back where they were and add new validator-specific tests that pass in input/rubric and receive a score.

handeyeco · 2024-09-18T15:45:21Z

packages/perseus/src/widgets/input-number/input-number-validator.test.ts

+        expect(err).toMatchInlineSnapshot(`
+            {
+              "message": "We could not understand your answer. Please check your answer for extra text or symbols.",
+              "type": "invalid",
+            }
+        `);
+    });


When we run into snapshot tests against objects, I'd advocate for switching them to .toEqual tests (non-snapshot).

jeremywiebe

Some duplicate comments from what Matthew already pointed out. I wrote these up yesterday and forgot to submit my review. :)

jeremywiebe · 2024-09-17T23:49:45Z

packages/perseus/src/widgets/input-number/input-number-validator.test.ts

+import {testDependencies} from "../../../../../testing/test-dependencies";
+import * as Dependencies from "../../dependencies";
+import {mockStrings} from "../../strings";
+import {renderQuestion} from "../__testutils__/renderQuestion";


These validator tests should ideally not have any dependency on React or renderQuestion. These will eventually be called in a server environment and so will be "headless".

I think some of these tests likely belong in input-number.test.tsx.

jeremywiebe · 2024-09-17T23:51:11Z

packages/perseus/src/widgets/input-number/input-number-validator.ts

+    state: {
+        currentValue: string;
+    },


Let's call this userInput and type it using the UserInput types that Matthew has created in #1639

Question: Should I wait for that PR to land then to reference UserInput or just copy over the PerseusInputNumberUserInput type for now?

Don't wait to land it. I can fix this in my PR.

jeremywiebe · 2024-09-17T23:55:25Z

packages/perseus/src/widgets/input-number/input-number-validator.ts

+    },
+    rubric: Rubric,
+    strings: PerseusStrings,
+    onInputError: APIOptions["onInputError"] = () => {},


This is going to be something we untangle. I don't think we'll have access to APIOptions in a server environment unless it directly affects scoring.

As such, we'll need to figure out how to communicate this all the way back to the caller.

Essentially, we won't be able to have callbacks in our validator functions.

I think we decided to handle this during a separate stage of the validation separation process, right? Should I just leave this here for now then? Maybe just adding a TODO to fix these when I find them would be the best thing for now

Don't do anything for now, we'll circle back. I think he was just thinking out loud.

handeyeco · 2024-09-19T14:52:06Z

packages/perseus/src/widgets/input-number/input-number-validator.test.ts

+
+import type {Rubric} from "./input-number.types";
+
+describe("static function validate", () => {


Suggested change

describe("static function validate", () => {

describe("inputNumberValidator", () => {

It's not a static method in this context.

Fixed! Thanks :)

handeyeco · 2024-09-19T14:52:56Z

packages/perseus/src/widgets/input-number/input-number-validator.test.ts

+    beforeEach(() => {
+        jest.spyOn(Dependencies, "getDependencies").mockReturnValue(
+            testDependencies,
+        );
+    });


We might need this, but I would try to run the tests without it and if they pass then remove it.

Already removed it :D And all the tests pass!

handeyeco · 2024-09-19T14:59:17Z

packages/perseus/src/widgets/input-number/input-number.test.ts

-} from "../../perseus-types";
+} from "@khanacademy/perseus";


That's weird, I think this should have been a linting error. 🤔 I think we have a linter that prevents packages from importing from themselves like this.

I don't know why the linter isn't mad, but I would revert this.

Already reverted it ^_^ Thank you! Trying to keep an eye out for things like this after you pointed out that strings import issue

@nishasy

This PR was opened by the [Changesets release](https://github.com/changesets/action) GitHub action. When you're ready to do a release, you can merge this and the packages will be published to npm automatically. If you're not ready to do a release yet, that's fine, whenever you add more changesets to main, this PR will be updated. # Releases ## @khanacademy/perseus@34.1.0 ### Minor Changes - [#1642](#1642) [`75e19c557`](75e19c5) Thanks [@nishasy](https://github.com/nishasy)! - [Locked Figure Labels] View locked line labels - [#1647](#1647) [`49bf45573`](49bf455) Thanks [@nishasy](https://github.com/nishasy)! - [Locked Figure Labels] Add labels to locked lines' defining points in the graph and editor - [#1644](#1644) [`136b6e54c`](136b6e5) Thanks [@nishasy](https://github.com/nishasy)! - [Locked Figure Labels] Add/edit/delete locked line labels ### Patch Changes - [#1646](#1646) [`7822ea63c`](7822ea6) Thanks [@nicolecomputer](https://github.com/nicolecomputer)! - Keyboard support for unlimited point graphs - [#1651](#1651) [`1080a628b`](1080a62) Thanks [@handeyeco](https://github.com/handeyeco)! - Consolidate Measurer and DeprecatedStandin to use noopValidator - [#1640](#1640) [`d766b33dd`](d766b33) Thanks [@Myranae](https://github.com/Myranae)! - Move validation logic out of the Input Number widget - [#1657](#1657) [`25d45af95`](25d45af) Thanks [@benchristel](https://github.com/benchristel)! - Internal: delete an outdated comment - [#1649](#1649) [`b5594e81d`](b5594e8) Thanks [@handeyeco](https://github.com/handeyeco)! - Custom Jest matchers for PerseusScore - [#1641](#1641) [`f5ceabb7d`](f5ceabb) Thanks [@Myranae](https://github.com/Myranae)! - Move validation logic out of the Categorizer widget - [#1636](#1636) [`64bcde0a1`](64bcde0) Thanks [@handeyeco](https://github.com/handeyeco)! - Small tweak to validation logic for non-interactive widgets ## @khanacademy/perseus-editor@14.4.0 ### Minor Changes - [#1642](#1642) [`75e19c557`](75e19c5) Thanks [@nishasy](https://github.com/nishasy)! - [Locked Figure Labels] View locked line labels - [#1647](#1647) [`49bf45573`](49bf455) Thanks [@nishasy](https://github.com/nishasy)! - [Locked Figure Labels] Add labels to locked lines' defining points in the graph and editor - [#1644](#1644) [`136b6e54c`](136b6e5) Thanks [@nishasy](https://github.com/nishasy)! - [Locked Figure Labels] Add/edit/delete locked line labels - [#1643](#1643) [`2950ec33f`](2950ec3) Thanks [@anniegallagher](https://github.com/anniegallagher)! - Make widget editors expanded by default. Add ability to expand/collapse all widget editors on a page externally. ### Patch Changes - Updated dependencies \[[`7822ea63c`](7822ea6), [`75e19c557`](75e19c5), [`49bf45573`](49bf455), [`1080a628b`](1080a62), [`d766b33dd`](d766b33), [`25d45af95`](25d45af), [`b5594e81d`](b5594e8), [`f5ceabb7d`](f5ceabb), [`136b6e54c`](136b6e5), [`64bcde0a1`](64bcde0)]: - @khanacademy/perseus@34.1.0 Author: khan-actions-bot Reviewers: jeremywiebe Required Reviewers: Approved By: jeremywiebe Checks: ✅ codecov/project, ✅ codecov/patch, ✅ Upload Coverage (ubuntu-latest, 20.x), ⏭️ Publish npm snapshot, ✅ Lint, Typecheck, Format, and Test (ubuntu-latest, 20.x), ✅ Jest Coverage (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), ✅ gerald Pull Request URL: #1648

Myranae added 4 commits September 17, 2024 15:51

Pull out the validate logic and reference new fx

099aded

Add related types to a new file

8855fc3

Move scoring related tests to new test file

0918550

Fix some import mistakes

996b6df

Myranae self-assigned this Sep 17, 2024

Add changeset

1b69559

Myranae commented Sep 17, 2024

View reviewed changes

Myranae marked this pull request as ready for review September 17, 2024 21:18

Myranae requested a review from handeyeco September 17, 2024 21:19

khan-actions-bot requested a review from a team September 17, 2024 21:19

Myranae requested review from a team and jeremywiebe and removed request for a team September 17, 2024 21:19

handeyeco requested changes Sep 18, 2024

View reviewed changes

jeremywiebe reviewed Sep 18, 2024

View reviewed changes

Myranae added 2 commits September 18, 2024 16:23

Return react tests and add basic ones

a150a03

Fix import and remove unneeded code

44435d2

handeyeco approved these changes Sep 19, 2024

View reviewed changes

Myranae added 2 commits September 19, 2024 10:02

Fix naming

250788e

Merge branch 'main' into tb/LEMS-2355/split-out-input-number-validate-fx

d7da108

Myranae merged commit d766b33 into main Sep 19, 2024
12 of 13 checks passed

Myranae deleted the tb/LEMS-2355/split-out-input-number-validate-fx branch September 19, 2024 15:22

khan-actions-bot mentioned this pull request Sep 19, 2024

Version Packages #1648

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Split out validation logic for input-number #1640

Split out validation logic for input-number #1640

Myranae commented Sep 17, 2024

github-actions bot commented Sep 17, 2024 •

edited

Loading

github-actions bot commented Sep 17, 2024 •

edited

Loading

codecov bot commented Sep 17, 2024 •

edited

Loading

Myranae Sep 17, 2024

handeyeco Sep 18, 2024

khan-actions-bot commented Sep 17, 2024

handeyeco left a comment

handeyeco Sep 18, 2024

handeyeco Sep 18, 2024

jeremywiebe left a comment

jeremywiebe Sep 17, 2024

jeremywiebe Sep 17, 2024

Myranae Sep 18, 2024

handeyeco Sep 19, 2024

jeremywiebe Sep 17, 2024

Myranae Sep 18, 2024

handeyeco Sep 19, 2024

handeyeco Sep 19, 2024

Myranae Sep 19, 2024

handeyeco Sep 19, 2024

Myranae Sep 19, 2024 •

edited

Loading

handeyeco Sep 19, 2024

Myranae Sep 19, 2024


		import type {Rubric} from "./input-number.types";

		describe("static function validate", () => {

	describe("static function validate", () => {
	describe("inputNumberValidator", () => {

Split out validation logic for input-number #1640

Split out validation logic for input-number #1640

Conversation

Myranae commented Sep 17, 2024

Summary:

Test plan:

github-actions bot commented Sep 17, 2024 • edited Loading

npm Snapshot: Published

github-actions bot commented Sep 17, 2024 • edited Loading

codecov bot commented Sep 17, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

khan-actions-bot commented Sep 17, 2024

Gerald

handeyeco left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeremywiebe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Myranae Sep 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Sep 17, 2024 •

edited

Loading

github-actions bot commented Sep 17, 2024 •

edited

Loading

codecov bot commented Sep 17, 2024 •

edited

Loading

Myranae Sep 19, 2024 •

edited

Loading