-
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] test/year_picker_test.test.js and some tests #4824
[typescript-migration] test/year_picker_test.test.js and some tests #4824
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.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #4824 +/- ##
=======================================
Coverage 96.67% 96.67%
=======================================
Files 28 28
Lines 2977 2977
Branches 1288 1285 -3
=======================================
Hits 2878 2878
Misses 99 99 ☔ View full report in Codecov by Sentry. |
669b8c0
to
cc867a8
Compare
const x = textContents.find( | ||
(year) => parseInt(year.textContent ?? "") === utils.getYear(minDate), | ||
); | ||
expect(x).not.toBeUndefined(); |
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.
It seemed that not
was needed to see the subsequent checklist items.
textContents.find( | ||
(year) => parseInt(year.textContent ?? "") === utils.getYear(minDate), | ||
), | ||
).not.toBeUndefined(); |
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.
It seemed that not
was needed to see the subsequent checklist items.
textContents.find( | ||
(year) => parseInt(year.textContent ?? "") === utils.getYear(maxDate), | ||
), | ||
).not.toBeUndefined(); |
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.
It seemed that not
was needed to see the subsequent checklist items.
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
+ 256
- 164
100% TSX (tests)
Type of change
Feature - These changes are adding a new feature or improvement to existing code.
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.
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.
Overall the changes look good and the safe access operators added should help avoid exceptions and still provide the correct test coverage. I added one comment regarding considering another approach to the tests, but what you have here should work fine as well. I didn't see any blocking issues. Thanks!
Reviewed with ❤️ by PullRequest
Nice work! |
name: Migrate year_picker_test.test.js and some tests
Description
Linked issue: #4700
Changes
year_picker_test.test.js
and some tests ware migrated to tsContribution checklist