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 tests for mixed number in KAS #1507

Merged
merged 14 commits into from
Aug 22, 2024
Merged

Add tests for mixed number in KAS #1507

merged 14 commits into from
Aug 22, 2024

Conversation

handeyeco
Copy link
Contributor

@handeyeco handeyeco commented Aug 9, 2024

Summary:

This is a failed attempt at LEMS-2198, but I want to keep the tests to help the next person who tries.

@handeyeco handeyeco self-assigned this Aug 9, 2024
@handeyeco handeyeco changed the title Mixed number handling in KAS WIP: Mixed number handling in KAS Aug 9, 2024
@khan-actions-bot khan-actions-bot requested a review from a team August 9, 2024 20:07
@khan-actions-bot
Copy link
Contributor

khan-actions-bot commented Aug 9, 2024

Gerald

Required Reviewers
  • @Khan/perseus for changes to .changeset/gentle-dingos-explain.md, packages/kas/src/__tests__/evaluating.test.ts, packages/kas/src/__tests__/parsing.test.ts

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

Copy link
Contributor

github-actions bot commented Aug 9, 2024

Size Change: +64 B (+0.01%)

Total Size: 856 kB

Filename Size Change
packages/perseus-editor/dist/es/index.js 277 kB +770 B (+0.28%)
packages/perseus/dist/es/index.js 414 kB -706 B (-0.17%)
ℹ️ 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 77.7 kB
packages/math-input/dist/es/strings.js 1.79 kB
packages/perseus-core/dist/es/index.js 1.48 kB
packages/perseus-linter/dist/es/index.js 21.8 kB
packages/perseus/dist/es/strings.js 3.29 kB
packages/pure-markdown/dist/es/index.js 3.67 kB
packages/simple-markdown/dist/es/index.js 12.4 kB

compressed-size-action

Copy link

codecov bot commented Aug 9, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 70.96%. Comparing base (6db145f) to head (a5cc8ce).
Report is 10 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1507      +/-   ##
==========================================
+ Coverage   70.07%   70.96%   +0.88%     
==========================================
  Files         515      519       +4     
  Lines      106234   106272      +38     
  Branches     7643    10942    +3299     
==========================================
+ Hits        74447    75412     +965     
+ Misses      31602    30860     -742     
+ Partials      185        0     -185     

Impacted file tree graph

see 157 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 6db145f...a5cc8ce. Read the comment docs.

Copy link
Contributor

github-actions bot commented Aug 14, 2024

npm Snapshot: Published

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

Example:

yarn add @khanacademy/perseus@PR1507

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

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

@handeyeco handeyeco mentioned this pull request Aug 15, 2024
handeyeco added a commit that referenced this pull request Aug 15, 2024
## Summary:
While working on [this PR](#1507), I noticed tests were passing when they shouldn't have been.

That PR might be a dead end, so I wanted to submit a fix in a separate PR.

Author: handeyeco

Reviewers: mark-fitzgerald

Required Reviewers:

Approved By: mark-fitzgerald

Checks: ✅ codecov/project, ✅ codecov/patch, ✅ Upload Coverage (ubuntu-latest, 20.x), ✅ Publish npm snapshot (ubuntu-latest, 20.x), ✅ Lint, Typecheck, Format, and Test (ubuntu-latest, 20.x), ✅ Check builds for changes in size (ubuntu-latest, 20.x), ✅ Jest Coverage (ubuntu-latest, 20.x), ✅ Cypress (ubuntu-latest, 20.x), ✅ Check for .changeset entries for all changed files (ubuntu-latest, 20.x), 🚫 Upload Coverage, ✅ gerald, 🚫 Publish npm snapshot (ubuntu-latest, 20.x), 🚫 Lint, Typecheck, Format, and Test (ubuntu-latest, 20.x), 🚫 Jest Coverage (ubuntu-latest, 20.x), 🚫 Check builds for changes in size (ubuntu-latest, 20.x), 🚫 Cypress (ubuntu-latest, 20.x), ✅ Check for .changeset entries for all changed files (ubuntu-latest, 20.x), 🚫 Upload Coverage, ✅ gerald, 🚫 Publish npm snapshot (ubuntu-latest, 20.x), 🚫 Lint, Typecheck, Format, and Test (ubuntu-latest, 20.x), 🚫 Cypress (ubuntu-latest, 20.x), 🚫 Jest Coverage (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), ✅ gerald

Pull Request URL: #1524
@kevinb-khan
Copy link
Contributor

Changes seem reasonable, but it looks like there's a few more test cases that could be added for this change.

@handeyeco handeyeco changed the title WIP: Mixed number handling in KAS Add tests for mixed number is KAS Aug 19, 2024
@handeyeco handeyeco changed the title Add tests for mixed number is KAS Add tests for mixed number in KAS Aug 19, 2024
Copy link
Contributor

@SonicScrewdriver SonicScrewdriver left a comment

Choose a reason for hiding this comment

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

Love seeing more tests, and thank you for calling out the TODO's with great comments. It really helped me quickly get context on the issue when coming in blind. This should definitely help with LEMS-2198!

@handeyeco handeyeco merged commit e19c58e into main Aug 22, 2024
19 checks passed
@handeyeco handeyeco deleted the mixed-numbers branch August 22, 2024 14:51
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.

5 participants