-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
[typescript-migration] some tests #4881
[typescript-migration] some tests #4881
Conversation
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.
✅ This pull request was sent to the PullRequest network for review. Expert reviewers are now being matched to your request based on the code's requirements. Stay tuned!
What to expect from this code review:
- Comments posted to any areas of potential concern or improvement.
- Detailed feedback or actions needed to resolve issues that are found.
- Turnaround times vary, but we aim to be swift.
@yuki0410-dev you can click here to see the review status or cancel the code review job.
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.
PullRequest Breakdown
Reviewable lines of change
+ 786
- 660
97% TSX (tests)
2% TSX
1% JavaScript
Type of change
Feature - These changes are adding a new feature or improvement to existing code.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4881 +/- ##
==========================================
- Coverage 96.96% 96.94% -0.03%
==========================================
Files 28 28
Lines 3234 3242 +8
Branches 1341 1332 -9
==========================================
+ Hits 3136 3143 +7
- Misses 94 99 +5
+ Partials 4 0 -4 ☔ View full report in Codecov by Sentry. |
7b2f82b
to
983eee4
Compare
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.
Looks good to me! These changes seem pretty simple, and I didn't spot any major issues. I did leave a few small suggestions, but nothing critical, particularly in light of most of the changes being to test code.
Ryan Lester <@buu700>
Reviewed with ❤️ by PullRequest
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.
The changes to leverage TypeScript more effectively look good overall. I added a note about the use of non-null assertions and in general suggest being cautious with those. They can mask real problems especially when used in tests and could make the tests less useful if the actual code stops providing the variable or data you are asserting should exist. Aside from that I don't see any security or other blocking issues to be concerned about. Thanks!
Reviewed with ❤️ by PullRequest
983eee4
to
1c8744f
Compare
1c8744f
to
3054d56
Compare
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.
Description
Linked issue: #4700
Changes
Contribution checklist