Skip to content
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: add more fixtures for strip-types #54107

Conversation

kevinuehara
Copy link
Contributor

@kevinuehara kevinuehara commented Jul 29, 2024

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: @tniessen @ErickWendel

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. test Issues and PRs related to the tests. labels Jul 29, 2024
@avivkeller avivkeller added the strip-types Issues or PRs related to strip-types support label Aug 9, 2024
@avivkeller
Copy link
Member

avivkeller commented Aug 9, 2024

Can you adjust the commit message to be prefixed with "test", and start with an active verb?

Basically change the beginning to "test: add". Preferably, "test: add more fixtures for strip-types"

@kevinuehara kevinuehara force-pushed the kevinuehara/adding-more-typescript-support-tests branch from ececa33 to 75ecd7e Compare August 9, 2024 14:23
@kevinuehara
Copy link
Contributor Author

Commit message updated @redyetidev <3 Thankss!

@marco-ippolito
Copy link
Member

Hi @kevinuehara I see fixtures being added, but these fixtures are not being tested

@kevinuehara
Copy link
Contributor Author

HI @marco-ippolito In fact, these fixtures are testing Typescript's own and unique functionalities (Utility Types). If it were not available there would be an error when compiling for Javascript so it can be considered a test and validation of the new ts support functionality

@marco-ippolito
Copy link
Member

marco-ippolito commented Aug 13, 2024

HI @marco-ippolito In fact, these fixtures are testing Typescript's own and unique functionalities (Utility Types). If it were not available there would be an error when compiling for Javascript so it can be considered a test and validation of the new ts support functionality

I'm not sure I understand. Where can I see those .ts files being executed or tested?
There should be a place where these .ts are run and the output is tested or checking they are executed correctly by node --experimental-strip-types.
For example in the original PR the file fixtures/typescript/test-typescript.ts is tested here

@marco-ippolito marco-ippolito changed the title [Tests] Adding More Tests For The Typescript initiative test: add more fixtures for strip-types Aug 13, 2024
Copy link

codecov bot commented Aug 13, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.77%. Comparing base (1d2603b) to head (ef24e34).
Report is 13 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #54107      +/-   ##
==========================================
- Coverage   88.06%   87.77%   -0.29%     
==========================================
  Files         651      651              
  Lines      183386   183386              
  Branches    35800    35478     -322     
==========================================
- Hits       161504   160976     -528     
- Misses      15159    15673     +514     
- Partials     6723     6737      +14     

see 71 files with indirect coverage changes

@kevinuehara
Copy link
Contributor Author

kevinuehara commented Sep 11, 2024

HI @marco-ippolito In fact, these fixtures are testing Typescript's own and unique functionalities (Utility Types). If it were not available there would be an error when compiling for Javascript so it can be considered a test and validation of the new ts support functionality

I'm not sure I understand. Where can I see those .ts files being executed or tested? There should be a place where these .ts are run and the output is tested or checking they are executed correctly by node --experimental-strip-types. For example in the original PR the file fixtures/typescript/test-typescript.ts is tested here

Hi @marco-ippolito! I update this PR with the tests

@kevinuehara
Copy link
Contributor Author

@redyetidev @marco-ippolito @ErickWendel can review this PR? 🙏

@marco-ippolito
Copy link
Member

can you please remove unrelated formatting changes?

test('expect error when executing a TypeScript file with generics', async () => {
const result = await spawnPromisified(process.execPath, [
'--experimental-strip-types',
fixtures.path('typescript/ts/test-generics.ts'),
Copy link
Member

Choose a reason for hiding this comment

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

if we are testing paramenter properties, why call it test-generics?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I call test-generics because I created the Typescript generics fixture. Do you have any suggestions for a name for the file?

Copy link
Member

Choose a reason for hiding this comment

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

test-parameter-properties?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good! Thanks! I will change the file name

strictEqual(result.code, 0);
});

test('execute a TypeScript file with Extract Union Type', async () => {
Copy link
Member

Choose a reason for hiding this comment

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

can you move the union test into one?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you say create a separate test file to test the union?

@avivkeller 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
Copy link
Member

also can you amend your first 2 commits with the right author?

@kevinuehara
Copy link
Contributor Author

can you please remove unrelated formatting changes?

I needed to format and break the line due to lint 🤔

@marco-ippolito
Copy link
Member

can you please remove unrelated formatting changes?

I needed to format and break the line due to lint 🤔

if it was there it means the lint has passed in the original PR 😄

@kevinuehara
Copy link
Contributor Author

@marco-ippolito I'll open a new PR with the updates.... I'm having trouble updating the author on the first two commits.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. 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
Development

Successfully merging this pull request may close these issues.

5 participants