[typescript-migration] some tests and configs #4891

yuki0410-dev · 2024-06-12T15:17:52Z

Description

Linked issue: #4700

Changes

migrate src/test/datepicker_test.test.tsx
migrate src/test/helper_components/custom_input.tsx
migrate rollup.config.mjs

Contribution checklist

I have followed the contributing guidelines.
I have added sufficient test coverage for my changes.
I have formatted my code with Prettier and checked for linting issues with ESLint for code readability.

pullrequest

⚠️ This pull request was not sent to the PullRequest network because the pull request is a draft.

codecov · 2024-06-12T15:20:21Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.65%. Comparing base (0728d1f) to head (0263278).

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #4891   +/-   ##
=======================================
  Coverage   96.64%   96.65%           
=======================================
  Files          28       28           
  Lines        3251     3254    +3     
  Branches     1354     1342   -12     
=======================================
+ Hits         3142     3145    +3     
- Misses        105      109    +4     
+ Partials        4        0    -4

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

pullrequest

✅ 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.

pullrequest

PullRequest Breakdown

Reviewable lines of change

+ 639
- 597

94% TSX (tests)
6% JavaScript
<1% GitHubActions (tests)

Type of change

Feature - These changes are adding a new feature or improvement to existing code.

martijnrusschen · 2024-06-12T17:19:13Z

.github/workflows/test.yml

@@ -30,8 +30,6 @@ jobs:
        with:
          path: ${{ steps.yarn-cache-dir-path.outputs.dir }}
          key: ${{ runner.os }}-yarn-${{ hashFiles('**/yarn.lock') }}
-          restore-keys: |


Can you expand on this?

@martijnrusschen
"restore-keys" searches cache by forward matching and uses the latest cache hit.
I have finished the ts-jest migration. (CI was ok).
I removed babel-jest as it is no longer needed. (CI was NG as the version dependency of the module used internally seemed to be broken.)
Drop the commit of ↑ (CI was NG as well as ↑)

From the above, I think it is a good idea to reduce CI time by using close cache as a base for yarn install, though,
However, if a cache with incorrect dependencies remains somewhere, there is a risk that subsequent CI will be NG, including other branches, so I thought it would be better to remove it.
What do you think?

Yes the intent of this cache is to reduce the CI time overall since it gets pretty slow if you don't have this. I haven't seen issues with cache hits before on dependency changes. I'd assume it would not match if there's a change in the yarn lock file since we use that in the key.

@martijnrusschen
Indeed, cache is created with hash and os in yarn.lock as keys.
However, according to the official documentation, if restore-keys is set, when using cache, it first searches for cache using hash and os in yarn.lock as keys; if cache is a hit, it is used, but if not, only os is used as key. If there is no hit, the repository is configured to use the most recent cache with only os as a key.
https://github.com/actions/cache?tab=readme-ov-file#inputs

I would expect that removing this setting will not slow down all PR CIs, but will make them take longer because cache is no longer allocated to PR CIs that include some yarn.lock changes.

The latest CI shows that cache is being used correctly without restore-keys as long as yarn.lock has not been changed.

https://github.com/Hacker0x01/react-datepicker/actions/runs/9486148892/job/26139812754?pr=4891

Run actions/cache@v4 Received 301[9](https://github.com/Hacker0x01/react-datepicker/actions/runs/9486148892/job/26139812754?pr=4891#step:5:10)89888 of 651797708 (46.3%), 287.1 MBs/sec Cache Size: ~622 MB (651797708 B) /usr/bin/tar -xf /home/runner/work/_temp/fbc7044b-c1cb-40eb-a8[10](https://github.com/Hacker0x01/react-datepicker/actions/runs/9486148892/job/26139812754?pr=4891#step:5:11)-6c1a5ad6255b/cache.tzst -P -C /home/runner/work/react-datepicker/react-datepicker --use-compress-program unzstd Received 651797708 of 651797708 (100.0%), 310.2 MBs/sec Cache restored successfully Cache restored from key: Linux-yarn-d94afddf929d782fe04721c5606e6ca65a0a1ee89384f90043903a1cc9339958

ok thanks for clarifying, that sounds like a good approach to me.

martijnrusschen

This is a major milestone! I think this marks the complete conversion to TS. Nice work!

pullrequest

All type annotations look correct; I have no concerns.

Jacob

Reviewed with ❤️ by PullRequest

pullrequest

Only one comment about the non-null assertion when coupled with the assert truthy - otherwise no concerns.

Jacques

Reviewed with ❤️ by PullRequest

src/test/datepicker_test.test.tsx

fix src/test/datepicker_test.test.tsx

a9f8df5

pullrequest bot reviewed Jun 12, 2024

View reviewed changes

yuki0410-dev force-pushed the feat/4700_ts-jest branch from 7dd9e49 to 6058bbc Compare June 12, 2024 15:50

yuki0410-dev changed the title ~~[typescript-migration] some tests~~ [typescript-migration] some tests and configs Jun 12, 2024

yuki0410-dev force-pushed the feat/4700_ts-jest branch 2 times, most recently from 6b70152 to 7416c44 Compare June 12, 2024 16:07

yuki0410-dev marked this pull request as ready for review June 12, 2024 16:17

pullrequest bot reviewed Jun 12, 2024

View reviewed changes

yuki0410-dev added 3 commits June 13, 2024 01:19

fix src/test/helper_components/custom_input.tsx

ff890c6

fix jest.config.js

2bf61a7

fix ci

0263278

yuki0410-dev force-pushed the feat/4700_ts-jest branch from e531b2d to 0263278 Compare June 12, 2024 16:19

martijnrusschen reviewed Jun 12, 2024

View reviewed changes

pullrequest bot reviewed Jun 12, 2024

View reviewed changes

pullrequest bot reviewed Jun 13, 2024

View reviewed changes

src/test/datepicker_test.test.tsx Show resolved Hide resolved

martijnrusschen merged commit 1cbe012 into Hacker0x01:main Jun 13, 2024
6 checks passed

yuki0410-dev deleted the feat/4700_ts-jest branch June 13, 2024 12:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[typescript-migration] some tests and configs #4891

[typescript-migration] some tests and configs #4891

yuki0410-dev commented Jun 12, 2024 •

edited

Loading

pullrequest bot left a comment

codecov bot commented Jun 12, 2024 •

edited

Loading

pullrequest bot left a comment

pullrequest bot left a comment •

edited

Loading

martijnrusschen Jun 12, 2024

yuki0410-dev Jun 12, 2024 •

edited

Loading

martijnrusschen Jun 13, 2024

yuki0410-dev Jun 13, 2024

yuki0410-dev Jun 13, 2024

martijnrusschen Jun 13, 2024

martijnrusschen left a comment

pullrequest bot left a comment

pullrequest bot left a comment

[typescript-migration] some tests and configs #4891

[typescript-migration] some tests and configs #4891

Conversation

yuki0410-dev commented Jun 12, 2024 • edited Loading

Description

Contribution checklist

pullrequest bot left a comment

Choose a reason for hiding this comment

codecov bot commented Jun 12, 2024 • edited Loading

Codecov Report

pullrequest bot left a comment

Choose a reason for hiding this comment

pullrequest bot left a comment • edited Loading

Choose a reason for hiding this comment

PullRequest Breakdown

martijnrusschen Jun 12, 2024

Choose a reason for hiding this comment

yuki0410-dev Jun 12, 2024 • edited Loading

Choose a reason for hiding this comment

martijnrusschen Jun 13, 2024

Choose a reason for hiding this comment

yuki0410-dev Jun 13, 2024

Choose a reason for hiding this comment

yuki0410-dev Jun 13, 2024

Choose a reason for hiding this comment

martijnrusschen Jun 13, 2024

Choose a reason for hiding this comment

martijnrusschen left a comment

Choose a reason for hiding this comment

pullrequest bot left a comment

Choose a reason for hiding this comment

pullrequest bot left a comment

Choose a reason for hiding this comment

yuki0410-dev commented Jun 12, 2024 •

edited

Loading

codecov bot commented Jun 12, 2024 •

edited

Loading

pullrequest bot left a comment •

edited

Loading

yuki0410-dev Jun 12, 2024 •

edited

Loading