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

tests(migration): typescript #613

Merged
merged 22 commits into from Mar 16, 2019
Merged

tests(migration): typescript #613

merged 22 commits into from Mar 16, 2019

Conversation

dhruvdutt
Copy link
Member

@dhruvdutt dhruvdutt commented Sep 28, 2018

What kind of change does this PR introduce?
Tests: Refactoring to TypeScript

Did you add tests for your changes?
Yes

If relevant, did you update the documentation?
NA

Summary
Added ts-test branch on upstream. We will keep working on this branch to migrate all tests to TypeScript.

Does this PR introduce a breaking change?
No

Other information
cc @sendilkumarn @ematipico

Packages that contains tests to port to typescript

@ematipico
Copy link
Contributor

Started the migration of other files. Please follow the same pattern

@rishabh3112
Copy link
Member

Can I help in this migration too?
If so can I get pointers from where to get started 😅

@ematipico ematipico force-pushed the ts-test branch 2 times, most recently from e89dde5 to 47d70e3 Compare October 1, 2018 06:49
@ematipico
Copy link
Contributor

ematipico commented Oct 1, 2018

Hi @rishabh3112, surely you can!
We are migrating the tests inside the different packages from JavaScript to TypeScript.

There's a task list that I added inside the main issue with the different packages and we are going to tick the ones that are migrated.

Please feel free to pick one and tell us, so we are going to update the main thread and we are not going to work on the same thing.

Most of them don't require an actual tests because they are just wrappers (add, update, etc.).

The main bulk will be utils package. So I'd suggest to take your time with one/two tests files and start the migration.

Please use the ts-test branch to commit your changes.

  • tests must be moved inside a __tests__ folder.
  • __fixtures__ will be moved inside the __tests__ folder
  • the javascript files inside the fixtures must stay as JavaScript file. They don't need to be ported because they are the real configuration that will be processed by our AST parser, hence TS is not a valid syntax

@rishabh3112
Copy link
Member

Thanks @ematipico 🙌,
I will look into this and let everyone know on which package I would be working.
(Sorry for late reply though 😅)

@rishabh3112
Copy link
Member

I am facing a little difficulty in setting up.
How can I keep intact my npm installation intact while working over repo?
@ematipico, @dhruvdutt, @ev1stensberg?

@evenstensberg
Copy link
Member

evenstensberg commented Oct 16, 2018

I think this PR should be on hold until the new test infrastructure is up

@dhruvdutt
Copy link
Member Author

Since the new infra is now ready, we can start working on this. Every contribution is welcomed. We will continue working on ts-test branch.

@rishabh3112 Not sure why GitHub isn't allowing me to assign you. 😅

Copy link
Member

@evenstensberg evenstensberg left a comment

Choose a reason for hiding this comment

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

Needs rebase

@misterdev
Copy link
Contributor

@dhruvdutt @evenstensberg I've worked on the pending requested changes and made a PR, would be great to have any kind of feedback 👍

tests(migration): including requested changes
@dhruvdutt
Copy link
Member Author

Working on rebasing this.

@evenstensberg
Copy link
Member

Status?

@webpack-bot
Copy link

@sendilkumarn Thanks for your update.

I labeled the Pull Request so reviewers will review it again.

@ematipico Please review the new changes.

@sendilkumarn
Copy link
Member

I merged upstream changes. Hopefully, this one will be good to go 🤞

@webpack-bot
Copy link

Thank you for your pull request! The most important CI builds succeeded, we’ll review the pull request soon.

package.json Outdated
@@ -43,7 +43,8 @@
"travis:integration": "npm run build && npm run test && npm run reportCoverage",
"travis:lint": "npm run build && npm run lint && npm run tslint",
"tslint": "tslint -c tslint.json \"packages/**/*.ts\"",
"watch": "npm run build && tsc -w"
"watch": "npm run build && tsc -w",
"test:local": "jest --watch"
Copy link
Member

Choose a reason for hiding this comment

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

could we remove this?

Copy link
Member

Choose a reason for hiding this comment

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

makes sense 👍

@evenstensberg evenstensberg changed the title [WIP] tests(migration): typescript tests(migration): typescript Mar 15, 2019
@evenstensberg
Copy link
Member

As a final check, could you pull master branch, check if we are missing something?

@sendilkumarn
Copy link
Member

This has latest master in it. so we can merge

@sendilkumarn
Copy link
Member

@evenstensberg Let us not wait further. I will merge this. I think on the Typescript side it is better to follow some conventions like utilizing type inferences and being strict on any (things like that). I will create a new PR out of this.

@ematipico ematipico merged commit 2106e70 into master Mar 16, 2019
@sendilkumarn sendilkumarn deleted the ts-test branch March 16, 2019 10:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants