Skip to content
This repository has been archived by the owner on Dec 10, 2020. It is now read-only.

Clean up getTests function, remove async/await library #38

Merged
merged 6 commits into from
Mar 20, 2019

Conversation

whymarrh
Copy link
Contributor

Fixes #37

This PR cleans up getTests, also replacing the async-await library with native async/await.

@holgerd77
Copy link
Member

CI is failing, can you also update this PR to run CI on Node 8, 10 and eventually 11?

@holgerd77
Copy link
Member

We should really add ethereumjs/ethereumjs-config#16 (Git hooks), I also stumble upon this regularly (linting errors). @whymarrh if you want you can open a small PR on this towards the blockchain or other library so that we get started, should be a pretty small one.

@whymarrh
Copy link
Contributor Author

CI is failing, can you also update this PR to run CI on Node 8, 10 and eventually 11?

On it

@whymarrh
Copy link
Contributor Author

@holgerd77 should be good now, I cleaned up the Travis config as well

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.

  1. These code changes are rather heavy. Can you please add a test case for this method, since "getTests" is completely untested atm, so that we can make sure that API functionality is working as before?
  2. Is Node 11 not working?
  3. Can you please re-add the JSDoc comment for the method?

@whymarrh
Copy link
Contributor Author

I added a test case for the default arguments, but I'm not quite sure how this is used in the wild—what other tests would be useful?

@rumkin
Copy link

rumkin commented Mar 20, 2019

While using async/await in source code it's better to set minimum node.js version to 8.0 in package.json:

{ "engines" : { "node" : ">=8.0" } }

And even better to define last tested version as an upper bound to avoid regression issues.

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, have also done a short integration test on the VM, looks good! 😄

@holgerd77 holgerd77 merged commit 19e2b2a into ethereumjs:master Mar 20, 2019
@holgerd77
Copy link
Member

holgerd77 commented Mar 20, 2019

Hmm, might have been merged too early/quickly, the integration tests I did on the VM show a huge decrease in coverage - see ethereumjs/ethereumjs-monorepo#477 - I think the blockchain tests are not run correctly (at all).

Can you have a look at that?

@whymarrh
Copy link
Contributor Author

Can you have a look at that?

I added a old commit hash to ethereumjs/ethereumjs-monorepo#477 so we have something to compare against. I can take a look at what's happening—this is weird because we have a test for methods that the vm is using that used to pass as well.

@holgerd77
Copy link
Member

You can have a look at the CI run, the state tests are just not running, respectively all skipped, this should likely be the cause for coverage decrease (see CircleCI coveralls run).

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