node icon indicating copy to clipboard operation
node copied to clipboard

test: adding more tests for strip-types

Open kevinuehara opened this issue 1 year ago • 21 comments

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

kevinuehara avatar Sep 13 '24 17:09 kevinuehara

Review requested:

  • [ ] @nodejs/typescript

nodejs-github-bot avatar Sep 13 '24 17:09 nodejs-github-bot

Again commits do not point to your github profile or email

marco-ippolito avatar Sep 13 '24 19:09 marco-ippolito

Again commits do not point to your github profile or email

The commits point to Kevin Uehara <[email protected]>, whereas your github profile's commits point to Gayathri <[email protected]>

avivkeller avatar Sep 13 '24 19:09 avivkeller

@marco-ippolito @RedYetiDev I update the author of commits... the ifood account is my work author :)

kevinuehara avatar Sep 13 '24 21:09 kevinuehara

@marco-ippolito how does this fixture compare to the other ones? Just want to make sure they are all similar

avivkeller avatar Sep 19 '24 11:09 avivkeller

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 avatar Sep 19 '24 11:09 marco-ippolito

@marco-ippolito @RedYetiDev Do you think we can approve and merge this PR?

kevinuehara avatar Sep 23 '24 13:09 kevinuehara

CI: https://ci.nodejs.org/job/node-test-pull-request/62690/

nodejs-github-bot avatar Sep 23 '24 14:09 nodejs-github-bot

@RedYetiDev can you review and approve the PR? 🙏

kevinuehara avatar Sep 23 '24 14:09 kevinuehara

@RedYetiDev can you review and approve the PR? 🙏

I'm not a core collaborator, so my reviews have no approving power. But, LGTM.

avivkeller avatar Sep 23 '24 14:09 avivkeller

@RedYetiDev can you review and approve the PR? 🙏

I'm not a core collaborator, so my reviews have no approving power. But, LGTM.

Thanks!! Now who can merge the PR?

kevinuehara avatar Sep 23 '24 15:09 kevinuehara

It's been labelled author ready. A maintainer can add commit-queue if they feel confident this should land.

avivkeller avatar Sep 23 '24 15:09 avivkeller

@kevinuehara can you rebase/squash the merge commit + resolve the lint error?

avivkeller avatar Sep 23 '24 15:09 avivkeller

@kevinuehara can you rebase/squash the merge commit + resolve the lint error?

Sorry! But can you help me with the rebase/squash the merge coomit? Is there any command to solve the lint problem?

kevinuehara avatar Sep 23 '24 15:09 kevinuehara

  1. make lint-js will lint, while make lint-js-fix will fix some of the errors
  2. git rebase -i HEAD~n, where n represents the number of commits, will open a UI where you can pick / squash commits. See https://www.freecodecamp.org/news/git-squash-commits/

avivkeller avatar Sep 23 '24 15:09 avivkeller

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 88.24%. Comparing base (c237eab) to head (240c9e9). Report is 405 commits behind head on main.

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     

see 24 files with indirect coverage changes

codecov[bot] avatar Sep 23 '24 16:09 codecov[bot]

@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?

kevinuehara avatar Sep 23 '24 17:09 kevinuehara

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)

avivkeller avatar Sep 23 '24 17:09 avivkeller

What's the issue you are having?

When I use the git rebase -i HEAD~2 all these commits appear due to the update with main:

image

kevinuehara avatar Sep 23 '24 17:09 kevinuehara

Oh okay. IIRC what you'll want to do is get the latest commit from main, and run

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 <commit> (c237eabf4c8f1d5ff6dfa95ae30930d6fc959d4e)

After this, re-add the changes from this PR. This can be done manually, or via git apply path/to/file.diff with the PR's diff file.

Once this is done, you can git add . and git commit

avivkeller avatar Sep 23 '24 17:09 avivkeller

@RedYetiDev I achieved! Thank you so much

kevinuehara avatar Sep 23 '24 18:09 kevinuehara

@RedYetiDev Is there anyone who can validate and merge the PR?

kevinuehara avatar Sep 30 '24 13:09 kevinuehara

CI: https://ci.nodejs.org/job/node-test-pull-request/62855/

nodejs-github-bot avatar Sep 30 '24 13:09 nodejs-github-bot

@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?

kevinuehara avatar Oct 01 '24 13:10 kevinuehara

@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

marco-ippolito avatar Oct 01 '24 13:10 marco-ippolito

CI: https://ci.nodejs.org/job/node-test-pull-request/62873/

nodejs-github-bot avatar Oct 01 '24 13:10 nodejs-github-bot

https://ci.nodejs.org/job/node-test-commit-arm/55143/nodes=rhel8-arm64/testReport/junit/(root)/parallel/test_stream_readable_unpipe_resume/ looks flaky.

avivkeller avatar Oct 01 '24 13:10 avivkeller

Thank you @marco-ippolito !!!

kevinuehara avatar Oct 01 '24 20:10 kevinuehara

Landed in 2755551c3c4c532579c39624ae886ea74753a686

nodejs-github-bot avatar Oct 02 '24 01:10 nodejs-github-bot