-
Notifications
You must be signed in to change notification settings - Fork 29.7k
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
test: adding more tests for strip-types #54929
test: adding more tests for strip-types #54929
Conversation
Review requested:
|
Again commits do not point to your github profile or email |
The commits point to |
0d4dde4
to
e1fbc08
Compare
@marco-ippolito @RedYetiDev I update the author of commits... the ifood account is my work author :) |
@marco-ippolito how does this fixture compare to the other ones? Just want to make sure they are all similar |
To be honest I dont think there is much value in testing TypeScript features such as Unions and Generics, since we rely on swc and swc tests that the output they produce is correct. Im ok with asserting that parameter properties throw in striptypes mode |
@marco-ippolito @RedYetiDev Do you think we can approve and merge this PR? |
ccfa065
to
f5ef1e0
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.
LGTM
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #54929 +/- ##
==========================================
- Coverage 88.26% 88.24% -0.02%
==========================================
Files 652 652
Lines 183898 183898
Branches 35859 35858 -1
==========================================
- Hits 162309 162286 -23
- Misses 14891 14898 +7
- Partials 6698 6714 +16 |
@RedYetiDev I'm having some troube to use the squash/merge... I'm thinking to close this PR and open a new one, what do you think? |
What's the issue you are having? git reset --soft HEAD~<n> # where <n> represents the number of commits, in this case, 9
git commit May also work. (See https://stackoverflow.com/questions/5189560/how-do-i-squash-my-last-n-commits-together) |
Oh okay. IIRC what you'll want to do is get the latest commit from git reset --hard <commit> # Right now it's c237eabf4c8f1d5ff6dfa95ae30930d6fc959d4e Caution This is a dangerous command. It'll remove all uncommitted changes, and revert your local workspace to look identical to After this, re-add the changes from this PR. This can be done manually, or via Once this is done, you can |
427b202
to
240c9e9
Compare
@RedYetiDev I achieved! Thank you so much |
@RedYetiDev Is there anyone who can validate and merge the PR? |
@RedYetiDev @marco-ippolito I saw that an error was occurring in the tests, but it was not one that I created or changed. Can you help? |
Its just flaky tests Ill re run the CI |
This comment was marked as duplicate.
This comment was marked as duplicate.
Thank you @marco-ippolito !!! |
Landed in 2755551 |
PR-URL: #54929 Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com> Reviewed-By: Michael Dawson <midawson@redhat.com>
PR-URL: #54929 Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com> Reviewed-By: Michael Dawson <midawson@redhat.com>
PR-URL: nodejs#54929 Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com> Reviewed-By: Michael Dawson <midawson@redhat.com>
In this MR I'm adding more tests created in this PR, testing generics and Utility Types.
This PR makes part of typescript iniciative on Node.
cc: @RedYetiDev @marco-ippolito @ErickWendel