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

Transition VM tests to TypeScript #881

Merged
merged 15 commits into from
Sep 20, 2020
Merged

Transition VM tests to TypeScript #881

merged 15 commits into from
Sep 20, 2020

Conversation

holgerd77
Copy link
Member

This PR transitions the VM test suite to TypeScript. State and blockchain tests are converted, API tests partially. First thought we could merge this partial API test transtion state cause the test:API command runs with both js and ts files but then realized that the test:API:browser command fails and it is probably easier to convert all the API tests as well and then adopt the Karma config and not introduce some hybrid setup.

I will (likely) stop on this for now. If you want to continue here feel free to do so, then please announce here before on what files you will be working and don't push directly on the branch but open a separate PR towards this branch.

@codecov
Copy link

codecov bot commented Sep 18, 2020

Codecov Report

Merging #881 into master will increase coverage by 1.88%.
The diff coverage is 100.00%.

Impacted file tree graph

Flag Coverage Δ
#account 92.85% <ø> (ø)
#block 77.03% <ø> (ø)
#blockchain 80.13% <ø> (ø)
#common 92.85% <ø> (ø)
#ethash 83.33% <ø> (ø)
#tx 93.98% <ø> (?)
#vm 87.64% <100.00%> (+5.45%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

@holgerd77
Copy link
Member Author

Note: this is now also getting under the reign of the linter, therefore there are more changes than the manual ones along the transition.

@jochem-brouwer
Copy link
Member

Have glanced over it, will have to take a deeper look later. I also love that tests are now being linted! 😄

@ryanio
Copy link
Contributor

ryanio commented Sep 18, 2020

woo thanks for starting this!

it looks like tests/api/EIPs/EIP2537-BLS-tests.js was erroring out with Cannot find module '../../util' since it was converted to ts, so i converted that file for now and can do some more conversion work in a separate PR as you suggested.

in working on that file I noticed that the vm.runCall opts are all taken in as Buffers so that would be some good future work to update them to better types (Address, BN, etc.)

i also updated the coverage:test script to accept ts

@evertonfraga
Copy link
Contributor

That's great! :D

As a following step, maybe we can keep a bare minimum of JS tests, to ensure the behavior of the API in the JS world. I realized how important this was when we had the discussion of undefined vs. null when instantiating classes. In the TS world it wouldn't compile, and in JS it would create a class with a weird state, with high risk of bugs down the road.

@ryanio
Copy link
Contributor

ryanio commented Sep 18, 2020

@evertonfraga that's a good idea, hm ok i was thinking of fully transitioning over to karma-typescript from karma-browserify but in that case if we want to keep some sanity JS tests we can probably use allowJs: true

ryanio
ryanio previously approved these changes Sep 20, 2020
Copy link
Contributor

@ryanio ryanio left a comment

Choose a reason for hiding this comment

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

ok, this looks good to me now!

  add .nycrc
  drop dist flag for tester
Copy link
Member Author

@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.

Woah, unbelievable PR, you really can't stop once you started, right? 😂

😁😄😆

Really great, will merge.

@holgerd77 holgerd77 merged commit e2e882d into master Sep 20, 2020
@holgerd77 holgerd77 deleted the vm-tests-typescript branch September 20, 2020 10:05
@jochem-brouwer
Copy link
Member

Love this PR and the lighting speed it was written!! 😍

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

4 participants