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

Remove dependencies on global settings #169

Merged
merged 7 commits into from
Oct 30, 2020
Merged

Conversation

Patrik-Stas
Copy link
Contributor

@Patrik-Stas Patrik-Stas commented Oct 23, 2020

This PR is refactoring a few things. It started with attempt to extract open_pool, init_core, open_wallet logic from api/ so that that these function would be located with other internal apis in src/*.rs. Having this done will enable start working on agent abstractions similar like what we have in agents/node/vcxagent-core
Many of wallet and pool tests started failing and it was difficult to figure out why as the logic in tests and utils/libindy/wallet was intertwined with reliance on global state in settings. Also when one test failed, it often caused domino effect as all wallet tests were using same wallet name. So I went down the rabbit hole and refactored the code a bit:

  • renamed some functions
  • removed some of the accesses to global state from utils/libindy and in tests. Instead, I made function interfaces more explicit. Where the function would previously read some data from global state (for example key derivation method), now it have to passed as argument
  • all tests are now using RAW KDF ( i believe some previously were not), so testing might be somewhat faster
  • Removed AgencyModeSetup for connection state machine tests - it was creating wallet but then mocking all calls into it - instead simple SetupIndyMocks works just fine for those tests.
  • Similarly SetupAriesMocks, SetupStrictAriesMocks could now also be replaced by SetupIndyMocks
  • SetupLibraryAgencyV1, SetupLibraryAgencyV1ZeroFees, SetupLibraryAgencyZeroFees were unused -> deleted
  • SetupWalletAndPool was deleted - test which needs to mock wallet and pool (test_init_with_file, test_init_with_config, test_vcx_init_with_default_values, test_vcx_init_called_twice_fails) use SetupWallet
    SetupPoolConfig to achieve the same result.

Signed-off-by: Patrik Stas patrik.stas@absa.africa

@Patrik-Stas
Copy link
Contributor Author

Let's not try to merge this before release though.

@Patrik-Stas Patrik-Stas force-pushed the refactor/remove-settings-deps branch 2 times, most recently from a02964a to 37cae1b Compare October 23, 2020 16:51
Signed-off-by: Patrik Stas <patrik.stas@absa.africa>
@codecov-io
Copy link

codecov-io commented Oct 23, 2020

Codecov Report

Merging #169 into master will increase coverage by 0.04%.
The diff coverage is 80.39%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #169      +/-   ##
==========================================
+ Coverage   51.06%   51.10%   +0.04%     
==========================================
  Files         150      151       +1     
  Lines       22957    22923      -34     
  Branches     6059     6053       -6     
==========================================
- Hits        11722    11714       -8     
+ Misses       7628     7603      -25     
+ Partials     3607     3606       -1     
Flag Coverage Δ
#unittests 51.10% <80.39%> (+0.04%) ⬆️

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

Impacted Files Coverage Δ
libvcx/src/aries/mod.rs 1.09% <0.00%> (-7.03%) ⬇️
libvcx/src/utils/libindy/mod.rs 88.23% <ø> (ø)
libvcx/src/utils/libindy/pool.rs 34.78% <0.00%> (+4.01%) ⬆️
libvcx/src/utils/devsetup.rs 27.62% <44.44%> (+1.28%) ⬆️
libvcx/src/init.rs 45.94% <45.94%> (ø)
libvcx/src/settings.rs 55.70% <66.66%> (-0.29%) ⬇️
libvcx/src/api/vcx.rs 59.92% <69.69%> (+2.15%) ⬆️
libvcx/src/messages/agent_utils.rs 52.29% <75.00%> (+0.32%) ⬆️
libvcx/src/utils/libindy/wallet.rs 67.83% <82.60%> (-0.29%) ⬇️
libvcx/src/api/connection.rs 39.87% <100.00%> (ø)
... and 38 more

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 28e1084...2c5458a. Read the comment docs.

Signed-off-by: Patrik Stas <patrik.stas@absa.africa>
Signed-off-by: Patrik Stas <patrik.stas@absa.africa>
Signed-off-by: Patrik Stas <patrik.stas@absa.africa>
libvcx/src/api/vcx.rs Show resolved Hide resolved
libvcx/src/api/wallet.rs Show resolved Hide resolved
libvcx/src/init.rs Outdated Show resolved Hide resolved
libvcx/src/messages/agent_utils.rs Outdated Show resolved Hide resolved
libvcx/src/messages/agent_utils.rs Show resolved Hide resolved
libvcx/src/init.rs Show resolved Hide resolved
Signed-off-by: Patrik Stas <patrik.stas@absa.africa>
@mirgee mirgee merged commit d45d75e into master Oct 30, 2020
@mirgee mirgee deleted the refactor/remove-settings-deps branch October 30, 2020 15:07
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