Skip to content
This repository has been archived by the owner on Apr 17, 2023. It is now read-only.

Re-factored tests #209

Closed
wants to merge 2 commits into from
Closed

Re-factored tests #209

wants to merge 2 commits into from

Conversation

dewstend
Copy link
Contributor

Description

Like mentioned in #192, I migrated the integration tests in offix-client to TypeScript and codeshifted mocha to Jest, some TypeScript type-checking is required, I'm sending this PR for suggestions and edits.

@darahayes
Copy link
Contributor

darahayes commented Oct 14, 2019

@dewstend Thanks so much for this amazing contribution! I'm really happy with the changes. The build is failing due to tonnes of linter errors which are not really your fault. We obviously wrote that JS code with different standards to how we write our TS code 😅

Fixing the lint issues will bring tonnes of extra changes to all those files but I think we should do it. You can run npm run lint-fix and push those additional changes and we will take it from there!

@wtrocki
Copy link
Contributor

wtrocki commented Oct 15, 2019

This is the most amazing change we received in entire hacktoberfest. Thank you so much. Do you mind to be added to our contributor list on our webpage?

@dewstend
Copy link
Contributor Author

@wtrocki, I don't mind being added to the contributors list! I represent @NovaHelm, I develop there.

I already linted the files, @darahayes, it was a bit unintuitive for me since you use TSLint and npm scripts.

I see there are .vscode files, I also use Code, and I usually have ESLint files and its plugin for auto-lint/format on save.
Also, as stated in palantir/tslint#4534, TSLint will be deprecated in favour of ESLint and typescript-eslint.

@darahayes
Copy link
Contributor

Hey @dewstend I still see some linter errors in the build. Would you be able to fix those so we can proceed with this PR?

I see there are .vscode files, I also use Code, and I usually have ESLint files and its plugin for auto-lint/format on save.

This is a completely valid approach, but our team decided to implement the linting process as an npm script that can be checked in CI.

Also, as stated in palantir/tslint#4534, TSLint will be deprecated in favour of ESLint and typescript-eslint.

Thanks a lot for this info. Offix started before TSLint became deprecated. I think we can solve that as a separate issue/PR if you are interested! As for this PR, I think we are good to go once the build is passing!

@dewstend
Copy link
Contributor Author

@darahayes I've tried troubleshooting some of the errors, I can't fix them with auto-linting, most errors are inherent to how code was written in JavaScript, when I ported to TypeScript automatically, I fixed what I could, other errors are specific to the design, maybe?

@wtrocki
Copy link
Contributor

wtrocki commented Oct 28, 2019

Thank you so much. Our team will take this over from now and merge PR as soon as possible

@darahayes
Copy link
Contributor

Working on getting this merged right now

@darahayes
Copy link
Contributor

@dewstend I have cherry picked your commits into #223 which includes some extra fixes. Once that PR is passing I will merge that one and close this one. Because you authored those commits you will still be added as a contributor to the project. Thanks for your work!

@darahayes
Copy link
Contributor

@dewstend your changes have landed in f5f501c

Thanks for your contribution!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants