-
Notifications
You must be signed in to change notification settings - Fork 350
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
Remove mock/test widgets #1577
Remove mock/test widgets #1577
Conversation
Size Change: -2.7 kB (-0.31%) Total Size: 859 kB
ℹ️ View Unchanged
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1577 +/- ##
==========================================
- Coverage 70.39% 70.30% -0.10%
==========================================
Files 554 571 +17
Lines 107555 111385 +3830
Branches 5530 11212 +5682
==========================================
+ Hits 75717 78309 +2592
- Misses 31722 33076 +1354
+ Partials 116 0 -116 see 1117 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
GeraldRequired Reviewers
Don't want to be involved in this pull request? Comment |
npm Snapshot: PublishedGood news!! We've packaged up the latest commit from this PR (af4b804) and published it to npm. You Example: yarn add @khanacademy/perseus@PR1577 If you are working in Khan Academy's webapp, you can run: ./dev/tools/bump_perseus_version.sh -t PR1577 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm definitely in favour of removing the example widgets, but I wonder if we want to keep the two mock widgets (or at least asset loading one) that are used in some of the tests. If we tie these tests to real widgets, we run the (possibly low) risk of breaking unrelated tests if we change the widget.
@@ -288,16 +288,22 @@ describe("server item renderer", () => { | |||
// render tree created. | |||
// Finally we re-render and poke the asset status to loaded. At that | |||
// everything is loaded. | |||
// | |||
// TODO: is it possible to use an existing widget to test the same logic? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thought: I think this mocked asset loading widget might be one we want to keep. The image
widget would be a widget that hooks into the asset loading context, but it may be tricky to assert that the ServerItemRenderer does the right thing using the implementation of a production widget, and not risk regression.
The implementation of that widget is extremely small. Also, I wonder if we could eliminate all the @ts-expect-error
suppressions by simply adding the mock widget to the list of widgets, but mark it as hidden: true
. Maybe that's a net negative though.
What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I came to the same conclusion as you: keep the widget. I chose to mark it as deprecated though so that people would at least be hesitant to use it (I personally think we should bias towards using real widgets when possible); that meant that I had to suppress "no deprecated" errors when using it.
I'd be fine removing the TODOs and deprecated stuff if you think that's best.
EDIT: I just went ahead and removed the deprecated stuff
## Summary: The only thing this PR does is it removes `sort-comp` ESLint exceptions and fixes the sort order. See: https://github.com/jsx-eslint/eslint-plugin-react/blob/master/docs/rules/sort-comp.md I didn't do: - `interactive-graph.tsx` since I didn't want to cause merge conflicts for that team - Components that will get deleted in [my other PR](#1577) Author: handeyeco Reviewers: jeremywiebe Required Reviewers: Approved By: jeremywiebe Checks: ✅ codecov/project, ✅ codecov/patch, ✅ Upload Coverage (ubuntu-latest, 20.x), ✅ Cypress (ubuntu-latest, 20.x), ✅ Publish npm snapshot (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), ✅ Lint, Typecheck, Format, and Test (ubuntu-latest, 20.x), ✅ Jest Coverage (ubuntu-latest, 20.x), ✅ Publish Storybook to Chromatic (ubuntu-latest, 20.x), ✅ gerald Pull Request URL: #1583
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/[email protected] ### Major Changes - [#1577](#1577) [`c875acd01`](c875acd) Thanks [@handeyeco](https://github.com/handeyeco)! - Remove example widgets and their editors ### Minor Changes - [#1570](#1570) [`c4432ffad`](c4432ff) Thanks [@nicolecomputer](https://github.com/nicolecomputer)! - Remove unlimited points via keyboard - [#1582](#1582) [`377b7ce68`](377b7ce) Thanks [@aemandine](https://github.com/aemandine)! - Add save warnings to PhET widget editor and un-hide widget from content editor widget dropdown ### Patch Changes - [#1578](#1578) [`78bb8573e`](78bb857) Thanks [@aemandine](https://github.com/aemandine)! - Remove simpleValidate from PhET widget - [#1585](#1585) [`a6ec402c0`](a6ec402) Thanks [@handeyeco](https://github.com/handeyeco)! - Reorganize files in the widgets folder - [#1589](#1589) [`d56952564`](d569525) Thanks [@aemandine](https://github.com/aemandine)! - Make PhET widget smaller - [#1587](#1587) [`8015cdefb`](8015cde) Thanks [@aemandine](https://github.com/aemandine)! - Tidying up PhET widget - [#1583](#1583) [`615567bd2`](615567b) Thanks [@handeyeco](https://github.com/handeyeco)! - Remove sort-comp exceptions and reorder components ## @khanacademy/[email protected] ### Major Changes - [#1577](#1577) [`c875acd01`](c875acd) Thanks [@handeyeco](https://github.com/handeyeco)! - Remove example widgets and their editors ### Minor Changes - [#1582](#1582) [`377b7ce68`](377b7ce) Thanks [@aemandine](https://github.com/aemandine)! - Add save warnings to PhET widget editor and un-hide widget from content editor widget dropdown ### Patch Changes - [#1585](#1585) [`a6ec402c0`](a6ec402) Thanks [@handeyeco](https://github.com/handeyeco)! - Reorganize files in the widgets folder - [#1587](#1587) [`8015cdefb`](8015cde) Thanks [@aemandine](https://github.com/aemandine)! - Tidying up PhET widget - [#1583](#1583) [`615567bd2`](615567b) Thanks [@handeyeco](https://github.com/handeyeco)! - Remove sort-comp exceptions and reorder components - Updated dependencies \[[`78bb8573e`](78bb857), [`a6ec402c0`](a6ec402), [`d56952564`](d569525), [`c875acd01`](c875acd), [`8015cdefb`](8015cde), [`c4432ffad`](c4432ff), [`615567bd2`](615567b), [`377b7ce68`](377b7ce)]: - @khanacademy/[email protected] ## @khanacademy/[email protected] ### Patch Changes - [#1585](#1585) [`a6ec402c0`](a6ec402) Thanks [@handeyeco](https://github.com/handeyeco)! - Reorganize files in the widgets folder
Summary:
I don't see the point of having a bunch of test widgets when we could just use real widgets in tests.
Open to push back.