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

Runs tests with ts-node instead of node. #654

Merged
merged 6 commits into from
Feb 6, 2020
Merged

Runs tests with ts-node instead of node. #654

merged 6 commits into from
Feb 6, 2020

Conversation

evertonfraga
Copy link
Contributor

This PR aims to both kill some technical debt from the TypeScript migration and normalize how tests are run, in preparation for the monorepo migration. This way we won't need to move dist files across CI builds.

The sentence below can still be found in our documentation, although after the TypeScript migration that wasn't possible anymore:
Tests run against source by default. They can be run with the --dist flag

@holgerd77 The API test runners are all hardcoded to require from dist/. With your OK, I can make it work just like blockchain and state tests: testing against the source by default and against dist/ using --dist.

@evertonfraga evertonfraga self-assigned this Jan 23, 2020
@evertonfraga evertonfraga requested review from s1na and holgerd77 and removed request for s1na January 23, 2020 22:28
@codecov
Copy link

codecov bot commented Jan 23, 2020

Codecov Report

Merging #654 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff           @@
##           master    #654   +/-   ##
======================================
  Coverage    91.4%   91.4%           
======================================
  Files          31      31           
  Lines        1978    1978           
  Branches      326     326           
======================================
  Hits         1808    1808           
  Misses         90      90           
  Partials       80      80
Flag Coverage Δ
#vm 91.4% <ø> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 32f6962...85bc0ee. Read the comment docs.

@holgerd77
Copy link
Member

@evertonfraga Yes, running on dist/ was a left-over from the TypeScript migration process, see comment here. This can be reverted.

@evertonfraga
Copy link
Contributor Author

Great! I will proceed with these changes. Thanks.

@holgerd77
Copy link
Member

@evertonfraga Ah, this is likely up again for review? Procedural note: better to just place another comment on stuff like this since GitHub is just not notifying on removing labels or stuff, then this is likely to be overlooked.

Have updated the branch and will do a review and eventually merge once tests are through again.

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.

Looks good, this PR makes also some smaller code corrections along the way. Mainly had a look through the package.json updates.

It also fixes a path bug when not using the --dist option in the blockchain test runner.

@holgerd77 holgerd77 merged commit 9e7dc98 into master Feb 6, 2020
@holgerd77 holgerd77 deleted the run-tests-ts branch February 6, 2020 09:36
@evertonfraga
Copy link
Contributor Author

Right, thanks!

There'll be a second step to this category of changes, which will completely remove require(../../dist) from tests. This does not seem crucial to the migration, so I put on hold for a bit.

Example: https://github.com/ethereumjs/ethereumjs-vm/search?q=require+dist&unscoped_q=require+dist

@holgerd77
Copy link
Member

@evertonfraga ok

@evertonfraga evertonfraga added this to the VM v5 milestone Jun 12, 2020
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.

2 participants