Skip to content
This repository has been archived by the owner on Jan 19, 2021. It is now read-only.

Upgrade test suite to TS #106

Merged
merged 3 commits into from
Mar 30, 2020
Merged

Upgrade test suite to TS #106

merged 3 commits into from
Mar 30, 2020

Conversation

ryanio
Copy link
Contributor

@ryanio ryanio commented Mar 26, 2020 •
edited
Loading

This PR:

  1. Upgrades test suite to TypeScript
  2. Adds new types BufferCallback and ProveCallback to satisfy implicit type errors in tests
  3. Removes use of deprecated new Buffer()
  4. Adds convenience script npm run benchmarks for previously existing benchmarks folder

@github-actions
Copy link

Coverage Status

Coverage remained the same at 94.493% when pulling 4a6ac39 on upgradeTestsToTS into b8c50ac on master.

@ryanio ryanio force-pushed the upgradeTestsToTS branch from 4a6ac39 to 5d75653 Compare March 26, 2020 23:58
@ryanio
Copy link
Contributor Author

ryanio commented Mar 27, 2020 •
edited
Loading

I added the file renames as a separate commit to try to encourage GitHub to show the files as renamed instead of all deleted and all inserted, but I guess it didn't work for some files 🤷🏻‍♂️sorry about that, makes the diff really challenging to compare. Let me know if you have suggestions about this.

For a better diff view, see just the one commit: Upgrade tests to TS

holgerd77
holgerd77 reviewed Mar 30, 2020
View reviewed changes
Copy link
Member

@holgerd77 holgerd77 left a comment

Choose a reason for hiding this comment

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

First look, some questions and clarifications.

benchmarks/checkpointing.ts Show resolved Hide resolved
package.json
@@ -48,6 +49,7 @@
],
"license": "MPL-2.0",
"dependencies": {
"@types/async": "^3.0.8",
"async": "^2.6.1",
Copy link
Member

Choose a reason for hiding this comment

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

We want at some point getting rid of the async dependencies - just as a side note - see issue here ethereumjs/organization#55

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah ok thanks

Copy link
Contributor Author

@ryanio ryanio Mar 30, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Oops this should be in devDeps, I’ll move it

Copy link
Member

Choose a reason for hiding this comment

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

Oh, you are right. Already merged, just do a short follow-up PR eventually.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No prob, will include in next PR for promisify work, hopefully today or tomorrow.

package.json Show resolved Hide resolved
package.json
@@ -8,6 +8,7 @@
"dist/**/*"
],
"scripts": {
"benchmarks": "npm run build && ts-node benchmarks/index.ts",
Copy link
Member

Choose a reason for hiding this comment

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

Ah, didn't see this when doing the comment on the benchmark files above.

However this fails for me with a completely fresh node module reinstall with several implicit any type errors. Any comment on that?

Copy link
Contributor Author

@ryanio ryanio Mar 30, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks, I just did a fresh install and ran into the errors, will look at fixing. edit: should be fixed with latest commit now

Copy link
Member

Choose a reason for hiding this comment

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

Can confirm that this works now.

src/baseTrie.ts Show resolved Hide resolved
holgerd77
holgerd77 approved these changes Mar 30, 2020
View reviewed changes
Copy link
Member

@holgerd77 holgerd77 left a comment

Choose a reason for hiding this comment

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

Ok, just skimmed through all the code parts. Most code changes are minor (type additions, syntax updates,..) and pretty much local, think we can very well merge here. Thanks a lot Ryan! 😄

Will directly do the merge.

benchmarks/random.js
const async = require('async')
import * as async from 'async'
import * as ethUtil from 'ethereumjs-util'
const Trie = require('../dist/index.js').BaseTrie
Copy link
Member

Choose a reason for hiding this comment

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

Just as a note: benchmarks now moved to be run on dist.

@holgerd77 holgerd77 merged commit 3b37de5 into master Mar 30, 2020
@holgerd77 holgerd77 deleted the upgradeTestsToTS branch March 30, 2020 20:35
@ryanio ryanio mentioned this pull request Apr 17, 2020
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@holgerd77 holgerd77 holgerd77 approved these changes

Assignees
No one assigned
Labels
PR state: needs review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ryanio @holgerd77