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 one regression test for radio widget parser #1871

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

benchristel
Copy link
Member

This PR is a "pilot" demonstrating how regression testing for the PerseusItem
parser will work. The regression test uses production data found by running the
exhaustive test tool.

Issue: LEMS-2582

Test plan:

yarn test

@khan-actions-bot
Copy link
Contributor

khan-actions-bot commented Nov 15, 2024

Gerald

Required Reviewers
  • @Khan/perseus for changes to .changeset/mean-pens-grow.md, packages/perseus/src/util/parse-perseus-json/perseus-parsers/radio-widget.ts, packages/perseus/src/util/parse-perseus-json/regression-tests/parse-perseus-json-regression.test.ts, packages/perseus/src/util/parse-perseus-json/regression-tests/data/radio-missing-noneOfTheAbove.json

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

Copy link
Member Author

@benchristel benchristel Nov 15, 2024

Choose a reason for hiding this comment

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

It's too bad JSON doesn't allow comments. I'd love it if we could include some indication of what bug/failure is being tested for by this file.

Maybe I could change the extension to .yaml? (YAML is a superset of JSON)

Oh, but then JSON.parse() will fail. :(

Copy link
Contributor

Choose a reason for hiding this comment

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

I would suggest a comment property, but since this tests for type checking, that would probably fail. Would it be worth adding an optional comment property as a valid field within all Perseus JSON - not in this PR, but maybe something at an opportune time?

Copy link
Contributor

github-actions bot commented Nov 15, 2024

npm Snapshot: Published

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

Example:

yarn add @khanacademy/perseus@PR1871

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

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

Copy link
Contributor

github-actions bot commented Nov 15, 2024

Size Change: 0 B

Total Size: 1.29 MB

ℹ️ View Unchanged
Filename Size
packages/kas/dist/es/index.js 38.9 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 77.8 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 699 kB
packages/perseus-linter/dist/es/index.js 22.2 kB
packages/perseus/dist/es/index.js 420 kB
packages/perseus/dist/es/strings.js 3.55 kB
packages/pure-markdown/dist/es/index.js 3.66 kB
packages/simple-markdown/dist/es/index.js 12.4 kB

compressed-size-action

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants