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

perf/integration tests: Preparing for precalculated wallet addresses lib feature #189

Merged
merged 42 commits into from
May 12, 2022

Conversation

tuliomir
Copy link
Contributor

@tuliomir tuliomir commented Apr 25, 2022

A new feature on the wallet-lib will allow wallets to have their addresses pre-calculated to save up on processing time at the integration tests.

This PR contains the code adaptations to use this feature, as designed by:

Once the wallet lib is updated with the address pre-calculation feature, the following average time improvements are expected:

  • Common wallets start time: from 9 seconds to 2.3 seconds
  • Multisig wallets start time: from 58 seconds to 4.6 seconds
    More details on this available on the 173 design.

With this change alone, runs will be executed in around 500 seconds, a gain of near 50% from the original ~1000 seconds.
The lib version bump will be made in a separate PR.

Summary of changes

  • Adds the pre calculated wallet addresses feature
    • Script for generating these wallets
    • Helper class for the main wallet logic
    • Refactored WalletHelper to use it
    • Mocked tests to cover the new code
  • Created benchmark classes to measure time in a more readable way:
    • Wallet initialization class
    • Transaction execution class
    • The test-utils-integration.js methods were refactored to use them
    • The WalletHelper.startMultipleWalletsForTest method was simplified
  • Refactored test constants into their own file
    • Some mocked tests started needing access to this constant data
  • Refactored common test functions into a core.util.js file
    • Avoid circular dependency in some cases
  • Developer scripts moved from src/utils to scripts/
    • Both for structure clarity and for keeping test coverages correct

Acceptance Criteria

  • All tests should pass with the current version of the wallet lib
  • All integration tests should use the new wallet pre-calculation feature

Security Checklist

  • Make sure you do not include new dependencies in the project unless strictly necessary and do not include dev-dependencies as production ones. More dependencies increase the possibility of one of them being hijacked and affecting us.

@tuliomir tuliomir added enhancement New feature or request tests labels Apr 25, 2022
@tuliomir tuliomir self-assigned this Apr 25, 2022
@lgtm-com
Copy link

lgtm-com bot commented Apr 25, 2022

This pull request introduces 1 alert when merging 475875b into ae1396d - view on LGTM.com

new alerts:

  • 1 for Unused variable, import, function or class

@codecov
Copy link

codecov bot commented Apr 25, 2022

Codecov Report

Merging #189 (314fdc9) into dev (ae1396d) will increase coverage by 0.48%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##              dev     #189      +/-   ##
==========================================
+ Coverage   85.84%   86.32%   +0.48%     
==========================================
  Files          19       18       -1     
  Lines         650      651       +1     
  Branches      133      134       +1     
==========================================
+ Hits          558      562       +4     
+ Misses         83       80       -3     
  Partials        9        9              
Impacted Files Coverage Δ
src/controllers/index.controller.js 74.69% <100.00%> (+2.19%) ⬆️
scripts/generate_words.js

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 ae1396d...314fdc9. Read the comment docs.

@tuliomir tuliomir marked this pull request as ready for review April 29, 2022 19:01
@tuliomir
Copy link
Contributor Author

tuliomir commented May 2, 2022

About the utils, I agree they should be moved to a scripts folder. This was done on ed0d3e1.

About the precalculation helper, that is still on the src folder, I'm having mixed ideas about what to do with it. I have a feeling it might be used on the application in the future, as some kind of helper lib or simulator actionable via http request.

  • If this proves to be true, it would need tests. I erred on the side of caution and made them already.
  • If it keeps being only a script helper, it could also be moved to the /scripts folder, along with its tests.

@tuliomir tuliomir requested a review from r4mmer May 2, 2022 20:37
@pedroferreira1 pedroferreira1 self-requested a review May 12, 2022 19:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants