FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

test: adding more tests for strip-types by kevinuehara · Pull Request #54929 · nodejs/node · GitHub

/ node Public

test: adding more tests for strip-types - #54929

Merged
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
kevinuehara:kevinuehara/adding-typescript-tests
Oct 2, 2024
ConversationCommits (1)ChecksFiles changed
Merged

test: adding more tests for strip-types#54929
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
kevinuehara:kevinuehara/adding-typescript-tests

Conversation

kevinuehara commented Sep 13, 2024

Copy link
Copy Markdown
Contributor

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

nodejs-github-bot commented Sep 13, 2024

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/typescript

nodejs-github-bot added esm Issues and PRs related to the ECMAScript Modules implementation. needs-ci PRs that need a full CI run. strip-types Issues or PRs related to strip-types support test Issues and PRs related to the tests. labels Sep 13, 2024
marco-ippolito reviewed Sep 13, 2024
View reviewed changes
Comment thread test/fixtures/typescript/ts/test-union-types.ts Outdated
Comment thread test/fixtures/typescript/ts/test-parameter-properties.ts Outdated
avivkeller reviewed Sep 13, 2024
View reviewed changes
Comment thread test/es-module/test-typescript.mjs Outdated
Comment thread test/es-module/test-typescript.mjs Outdated
avivkeller added the commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. label Sep 13, 2024

marco-ippolito commented Sep 13, 2024

Copy link
Copy Markdown
Member

Again commits do not point to your github profile or email

avivkeller commented Sep 13, 2024

Copy link
Copy Markdown
Member

Again commits do not point to your github profile or email

The commits point to Kevin Uehara <kevin.uehara@ifood.com.br>, whereas your github profile's commits point to Gayathri <gayathriraj.vlr@gmail.com>

kevinuehara force-pushed the kevinuehara/adding-typescript-tests branch from 0d4dde4 to e1fbc08 Compare September 13, 2024 21:16

kevinuehara commented Sep 13, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

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

avivkeller reviewed Sep 15, 2024
View reviewed changes
Comment thread test/es-module/test-typescript.mjs Outdated
Comment thread test/es-module/test-typescript.mjs Outdated
avivkeller reviewed Sep 15, 2024
View reviewed changes
Comment thread test/es-module/test-typescript.mjs Outdated
Comment thread test/fixtures/typescript/ts/test-union-types.ts Outdated
avivkeller reviewed Sep 19, 2024
View reviewed changes
Comment thread test/es-module/test-typescript.mjs Outdated

avivkeller commented Sep 19, 2024

Copy link
Copy Markdown
Member

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

marco-ippolito commented Sep 19, 2024 •
edited
Loading

Copy link
Copy Markdown
Member

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

kevinuehara commented Sep 23, 2024

Copy link
Copy Markdown
Contributor Author

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

marco-ippolito added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 23, 2024
github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 23, 2024

nodejs-github-bot commented Sep 23, 2024

Copy link
Copy Markdown
Collaborator

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

avivkeller added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Sep 23, 2024
kevinuehara force-pushed the kevinuehara/adding-typescript-tests branch from ccfa065 to f5ef1e0 Compare September 23, 2024 14:39
mhdawson approved these changes Sep 23, 2024
View reviewed changes

mhdawson left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM

avivkeller commented Sep 23, 2024 •
edited
Loading

Copy link
Copy Markdown
Member

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)

kevinuehara commented Sep 23, 2024

Copy link
Copy Markdown
Contributor Author

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:

avivkeller commented Sep 23, 2024 •
edited
Loading

Copy link
Copy Markdown
Member

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> (c237eab)

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

kevinuehara force-pushed the kevinuehara/adding-typescript-tests branch from 427b202 to 240c9e9 Compare September 23, 2024 18:00

kevinuehara commented Sep 23, 2024

Copy link
Copy Markdown
Contributor Author

@redyetidev I achieved! Thank you so much

kevinuehara commented Sep 30, 2024

Copy link
Copy Markdown
Contributor Author

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

avivkeller added author ready PRs that have at least one approval, no outstanding review comments, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Sep 30, 2024
github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 30, 2024

nodejs-github-bot commented Sep 30, 2024

Copy link
Copy Markdown
Collaborator

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

kevinuehara commented Oct 1, 2024

Copy link
Copy Markdown
Contributor Author

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

marco-ippolito commented Oct 1, 2024

Copy link
Copy Markdown
Member

@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

nodejs-github-bot commented Oct 1, 2024

Copy link
Copy Markdown
Collaborator

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

This comment was marked as duplicate.

Sign in to view

kevinuehara commented Oct 1, 2024

Copy link
Copy Markdown
Contributor Author

Thank you @marco-ippolito !!!

marco-ippolito added commit-queue Add this label to land a pull request using GitHub Actions. and removed commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels Oct 2, 2024
nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 2, 2024
nodejs-github-bot merged commit 2755551 into nodejs:main Oct 2, 2024

nodejs-github-bot commented Oct 2, 2024

Copy link
Copy Markdown
Collaborator

Landed in 2755551

This was referenced Oct 2, 2024
Closed
Closed
targos pushed a commit that referenced this pull request Oct 4, 2024
test: adding more tests for strip-types
2e8d105 
PR-URL: #54929
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
targos pushed a commit that referenced this pull request Oct 4, 2024
test: adding more tests for strip-types
ce67e7b 
PR-URL: #54929
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
aduh95 mentioned this pull request Oct 9, 2024
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

avivkeller avivkeller left review comments
marco-ippolito marco-ippolito approved these changes
+1 more reviewer
mhdawson mhdawson approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no outstanding review comments, and a CI started. esm Issues and PRs related to the ECMAScript Modules implementation. needs-ci PRs that need a full CI run. strip-types Issues or PRs related to strip-types support test Issues and PRs related to the tests.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

Back | FazBrowse Home | New Git URL