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

Migrate all tests code to TypeScript #2609

Closed
wants to merge 1 commit into from
Closed

Migrate all tests code to TypeScript #2609

wants to merge 1 commit into from

Conversation

dotansimha
Copy link
Member

@dotansimha dotansimha commented Jun 4, 2020

This PR is based on the TypeScript migration path from #2104

This PR is also inspired by #2139 , but it uses the most recent codebase, and migrates all tests files to TS.

The changes in this PR includes the following:

TS Migration

  • Migrate all tests code to use TypeScript, and added some missing .d.ts files for test utils and other .js files that are in use by both Flow code and TypeScript tests.
  • Added root tsconfig.json, this is needed because we are going to compile TypeScript files, and the tsconfig.json that located under src is different, and used by dtslint only.
  • Added ts-node and use ts-node/register in Mocha configuration to support .ts files.

@dotansimha dotansimha mentioned this pull request Jun 4, 2020
6 tasks
@IvanGoncharov IvanGoncharov added the PR: polish 💅 PR doesn't change public API or any observed behaviour label Jun 4, 2020
@IvanGoncharov
Copy link
Member

@dotansimha Looks gorgeous, thanks a lot 👍
Can you please rebase it?

Bug fixes (non-breaking)

Can you please make it a separate PR?

@IvanGoncharov
Copy link
Member

Lint

@dotansimha You can turn off specific rules just for tests see bottom of .eslintrc.yml

@IvanGoncharov
Copy link
Member

IvanGoncharov commented Jun 4, 2020

remove dtslist and tslint)

@dotansimha We don't use tslint our self it's used only dtslint and we can't remove dtslint since we need to test our typings with different versions of typescript and also ensure that typings are properly exposed through package.json.

But AFAIK dtslint plans to migrate to eslint anyway.

@dotansimha
Copy link
Member Author

@IvanGoncharov Working on a rebase now, I saw you changed some of the eslint/tslint rules so I'm adjusting to it.
Regarding bug fixes - can we include this as part of this PR? some of those are needed in order to make the TS tests compile correctly...

@dotansimha
Copy link
Member Author

dotansimha commented Jun 4, 2020

@IvanGoncharov rebased and pushed fixes for all notes :)

package.json Outdated Show resolved Hide resolved
@IvanGoncharov
Copy link
Member

@dotansimha Also, don't worry about rebasing I will temporarily stop committing or merging prs for the next couple of days.

@IvanGoncharov
Copy link
Member

Regarding bug fixes - can we include this as part of this PR? some of those are needed in order to make the TS tests compile correctly...

@dotansimha I understand.
Please submit a separate PR and I will merge it and you will rebase this one.
It will also help if in this new PR you can provide examples of what's wrong with current typings as comments on code but only if fix is not obvious.

Also, can you please go through your changes split other stuff not directly related to TS conversion e.g. .nested.deep to .deep.nested conversion.
I will happily merge it as quick PRs and you can rebase.

It will save a lot of time on back and forth comments between us and will make my life so much easier because I need to review every single line and my head is exploding 🤯

@IvanGoncharov
Copy link
Member

IvanGoncharov commented Jun 4, 2020

@dotansimha Need to merge some PRs but will try to not touch tests.
If you need help I can help extracting *.d.ts fixes into separate PR?

@dotansimha
Copy link
Member Author

dotansimha commented Jun 5, 2020

@IvanGoncharov moved all d.ts changes to a new PR: #2616

I think you can merge other PRs if you need to, I'll need a rebase anyway after merging the #2616.

Regarding the deep.nested changed, it's actually does related to the TS migration, because the Chai/Mocha types does have nested.deep, only deep.nested.

@dotansimha
Copy link
Member Author

@IvanGoncharov done, but I'm not sure why codecov still complains about missing coverage? I can't see there anything missing. Am I missing something?

@IvanGoncharov
Copy link
Member

but I'm not sure why codecov still complains about missing coverage? I can't see there anything missing. Am I missing something?

@dotansimha Investigating this at the moment, think I know the solution would fix it on branch.
Meanwhile can you please address this comment #2616 (comment)?

IvanGoncharov added a commit to IvanGoncharov/graphql-js that referenced this pull request Jun 6, 2020
IvanGoncharov added a commit to IvanGoncharov/graphql-js that referenced this pull request Jun 6, 2020
IvanGoncharov added a commit that referenced this pull request Jun 6, 2020
@dotansimha
Copy link
Member Author

dotansimha commented Jun 7, 2020

@IvanGoncharov done :) So I think the next steps is to merge #2616 and #2624 , then rebase this one and make sure coverage works, right?

@IvanGoncharov
Copy link
Member

IvanGoncharov commented Jun 7, 2020

@dotansimha Changes are too big and I can constantly discover new issues in this PR.
I spend hours on it and I went only through ~10% and it's a total mix of different issues, even list of files is off
image
It's a good proof of concept but in current form, if I start commenting on every issue it will take ages of back forth.
So let's try a different approach, I created https://github.com/graphql/graphql-js/tree/ts_tests branch for accepting small topic PRs that can be merged after very short review.

First, such PR should disable all falling eslint rules and disable all failing checks in tsconfig.json focus on converting to TS syntax only and it's totally ok to merge it if it fails coverage or any other check that can't be disabled for converted files.
No need to add new *.d.ts files and no need to enable type check with tsc just focus on converting syntax and running npm run testonly.
But please review it yourself first to ensure it contains only correct files and only syntax changes.

Meanwhile, I will work on improving the situation with coverage and also removing src/tsconfig.json and other dtslint related stuff.

@dotansimha
Copy link
Member Author

I see what you mean @IvanGoncharov , thank you. Please note that most files has been transformed to TS in terms of syntax, but some needed additional minor changes to support TS. We covered the rest of the changes and we understood why they are needed (additional tsconfig, changes to eslint, changes to cspell and a fix for codecov - all the rest are just transformation from Flow to TS).

Personally, I don't really see the benefit of 10 separate PRs over 1 working PR that already exists, especially if they eventually do the same and will go into the same target branch (instead of master), but I'm willing to do those changes :)

It just feels like we'll always be in a state of "moving to TS" instead of just do it...

What do you think I should start with? what should the first PR contain and how we should gradually move there?

@dotansimha
Copy link
Member Author

@IvanGoncharov I created this: #2634 , starting to gradually make it compatible.
I still think that merging this PR into ts_tests could work and make us closer to TS migration, especially because you already reviewed most of it, and we had a few iterations on it.

@IvanGoncharov
Copy link
Member

@dotansimha I'm ok to merge syntax changes together with disabling ESLint rules as described in my previous comment:

First, such PR should disable all falling eslint rules and disable all failing checks in tsconfig.json focus on converting to TS syntax only and it's totally ok to merge it if it fails coverage or any other check that can't be disabled for converted files.
No need to add new *.d.ts files and no need to enable type check with tsc just focus on converting syntax and running npm run testonly.
But please review it yourself first to ensure it contains only correct files and only syntax changes.

Please disable rules only if they are failing on syntax changes.
No need to add ! or as just disable noImplicitAny.
Also please review your own PRs line by line before submitting them and ensure they contain only syntax changes.

I still think that merging this PR into ts_tests could work and make us closer to TS migration, especially because you already reviewed most of it, and we had a few iterations on it.

I reviewed only 20% of changes
image

PR that contains only syntax changes can be reviewed in less than an hour.

IvanGoncharov added a commit to IvanGoncharov/graphql-js that referenced this pull request Jun 14, 2020
IvanGoncharov added a commit to IvanGoncharov/graphql-js that referenced this pull request Jun 14, 2020
IvanGoncharov added a commit to IvanGoncharov/graphql-js that referenced this pull request Jun 15, 2020
IvanGoncharov added a commit to IvanGoncharov/graphql-js that referenced this pull request Jun 15, 2020
IvanGoncharov added a commit that referenced this pull request Jun 15, 2020
@dotansimha dotansimha mentioned this pull request Dec 1, 2020
17 tasks
Base automatically changed from master to main January 27, 2021 11:10
@yaacovCR
Copy link
Contributor

This can be closed considering that the code base has been converted to TS.

Thank you @dotansimha for your work on this, which has been integrated into other PRs, as well as for your energy and initiative in pushing forward the migration.

@yaacovCR yaacovCR closed this Oct 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: polish 💅 PR doesn't change public API or any observed behaviour
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants