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

Refactor test runner #752

Merged
merged 2 commits into from
May 25, 2020
Merged

Refactor test runner #752

merged 2 commits into from
May 25, 2020

Conversation

holgerd77
Copy link
Member

@holgerd77 holgerd77 commented May 23, 2020

Long overdue refactoring of the test runner. Sorry, this was such a mess and there was so much interdependent to correct that I gave up on keeping the changes separate and put everything in one commit. Hope this is somewhat reviewable. This should also proof to a greater extend correct if the CI runs pass, I will keep this in draft state for some more days and also do some more local failure case testing + testing of CL parameters and report back here.

PR does the following:

  • Promisification of test runners + (most) of the util functions
  • Separate config in tests/config.js
  • Broader code cleanup
  • Removal of VMTests since not being used for years and being deprecated now for some time on ethereum/tests
  • Removal of async library usage (some missing in util.js)
  • Update: fixed a bug where exceptions in blockchain tests went unnoticed, these now either failed or are compared to an eventually present expectException[FORKNAME] field in the test file
  • Adds two shell scripts for manually testing CI options for blockchain and state tests
  • Temporarily add (extremely extensive) ForkStressTest blockchain test to skipped tests, should be addressed in a separate PR

@codecov
Copy link

codecov bot commented May 23, 2020

Codecov Report

Merging #752 into master will increase coverage by 0.44%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #752      +/-   ##
==========================================
+ Coverage   91.63%   92.08%   +0.44%     
==========================================
  Files          45       46       +1     
  Lines        2964     3006      +42     
  Branches      467      468       +1     
==========================================
+ Hits         2716     2768      +52     
+ Misses        150      144       -6     
+ Partials       98       94       -4     
Flag Coverage Δ
#account 94.11% <ø> (?)
#block 88.36% <ø> (+0.27%) ⬆️
#blockchain 89.15% <ø> (+0.41%) ⬆️
#common 94.37% <ø> (ø)
#tx 94.02% <ø> (ø)
#vm 93.09% <ø> (+0.56%) ⬆️
Impacted Files Coverage Δ
packages/account/src/index.ts 94.11% <0.00%> (ø)
packages/vm/lib/evm/opFns.ts 89.98% <0.00%> (+0.37%) ⬆️
packages/vm/lib/evm/precompiles/09-blake2f.ts 98.14% <0.00%> (+1.85%) ⬆️
packages/vm/lib/evm/precompiles/05-modexp.ts 95.12% <0.00%> (+8.53%) ⬆️
packages/blockchain/src/cache.ts 100.00% <0.00%> (+20.00%) ⬆️

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 0333e31...778d1a4. Read the comment docs.

@holgerd77 holgerd77 force-pushed the refactor-test-runner branch 2 times, most recently from 958b34a to 3b5f1fa Compare May 23, 2020 16:07
@evertonfraga
Copy link
Contributor

All green!
Cleaning old code and modernizing it was much needed.

@holgerd77
Copy link
Member Author

Phew, did a lot of CI runs on this. I think I now have got it, will give a last ok once all the tests pass. I rolled back on promisification of util.setupPreconditions since this caused selected failures on all test types (API, StateTests, BlockchainTests) and I couldn't draw conclusions on the underlying root cause. So this can be tackled separately eventually.

The last failed test runs of the blockchain tests were actually some previously hidden failure runs by passing on errors in places like here but never do anything with it. I have now fixed this, this was happening on a lot of test where there were expectException[FORKNAME] fields in the test files, so in the current run these errors are now detected and will pass if such a field is present in the test file.

@holgerd77 holgerd77 force-pushed the refactor-test-runner branch 3 times, most recently from d349085 to b748290 Compare May 24, 2020 15:34
@holgerd77 holgerd77 force-pushed the refactor-test-runner branch from b748290 to c441719 Compare May 24, 2020 16:18
@holgerd77
Copy link
Member Author

Ok, finally. All VM tests pass (I think the Block test failure can be safely ignored, this is just due to some GH Actions outage likely). So this is now ready for review. 😀

I added two additional tests to the skip list, these were exposed by the expectException feature integration and can/should be handled separately.

I also added two simple scripts for manually testing/triggering various CL options for testing the test runner a bit more systematically.

@holgerd77
Copy link
Member Author

Some more notes:

The command executions from the two scripts pass apparently.

The test exception declaration awareness is rather just a start than a final integration. At some point it might be worth to have a closer look again here if exceptions are actually thrown within an appropriate context and handled appropriately. Should nevertheless be some working ground for this round here.

@ryanio
Copy link
Contributor

ryanio commented May 25, 2020

looking great 🎉 added a small commit to remove the test:vm package.json script

@holgerd77
Copy link
Member Author

@ryanio Thanks 😉, can you already give an approval or do you want to wait a bit longer?

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.

lgtm!

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.

3 participants