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/remove mocks #1005

Merged
merged 17 commits into from
Oct 4, 2023
Merged

Refactor/remove mocks #1005

merged 17 commits into from
Oct 4, 2023

Conversation

bobozaur
Copy link
Contributor

@bobozaur bobozaur commented Oct 2, 2023

There are plenty of occurrences where some occult mocking mechanism is employed by holding some global settings HashMap and setting/getting values from it.

This PR explores and aims to deal with this in multiple ways:

  • remove this completely if possible
  • adjust the mocks to only be employed when testing through #[cfg(test)]

This sets the ground for better decoupling the test code from production code.

Update

Unfortunately conditionally compiling mocks instead of real components based on the test profile does not work and neither does using real components in all tests. The reason is because with the runtime global flags there were tests that were using the mocks while others were not.

The problems were arising in the libvcx_core tests and the node JS wrapper tests. The tests were commented out to allow the CI to pass. In my opinion, the tests were awkward in the first place because:

  • they test stuff that's already tested in aries_vcx (with real components)
  • as higher level libraries, they rely on mocks, while the testing in aries_vcx does not
  • there are e2e tests in the node JS wrapper which do a better job than those pseudo-integration mocked tests

Patrik-Stas
Patrik-Stas previously approved these changes Oct 2, 2023
@codecov-commenter
Copy link

codecov-commenter commented Oct 3, 2023

Codecov Report

Merging #1005 (834d877) into main (ff6a060) will increase coverage by 0.11%.
The diff coverage is 80.24%.

❗ Current head 834d877 differs from pull request most recent head 1fddbde. Consider uploading reports for the commit 1fddbde to get more accurate results

@@            Coverage Diff             @@
##             main    #1005      +/-   ##
==========================================
+ Coverage   30.39%   30.50%   +0.11%     
==========================================
  Files         422      420       -2     
  Lines       26281    26005     -276     
  Branches     5100     5022      -78     
==========================================
- Hits         7987     7932      -55     
+ Misses      16070    15890     -180     
+ Partials     2224     2183      -41     
Flag Coverage Δ
unittests-aries-vcx 30.50% <80.24%> (+0.11%) ⬆️

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

Files Coverage Δ
aries_vcx/src/common/anoncreds.rs 82.88% <ø> (ø)
aries_vcx/src/common/credentials/mod.rs 81.81% <ø> (ø)
aries_vcx/src/common/ledger/transactions.rs 49.61% <ø> (+0.38%) ⬆️
...vcx/src/common/primitives/credential_definition.rs 58.44% <ø> (+0.99%) ⬆️
...ies_vcx/src/common/primitives/credential_schema.rs 11.57% <ø> (-5.61%) ⬇️
...s_vcx/src/common/primitives/revocation_registry.rs 52.52% <ø> (+1.54%) ⬆️
...es_vcx/src/common/proofs/prover/prover_internal.rs 26.49% <ø> (ø)
...cx/src/common/proofs/verifier/verifier_internal.rs 34.64% <ø> (+0.30%) ⬆️
aries_vcx/src/core/profile/mod.rs 0.00% <ø> (ø)
aries_vcx/src/core/profile/modular_libs_profile.rs 65.38% <ø> (ø)
... and 34 more

... and 4 files with indirect coverage changes

@bobozaur bobozaur force-pushed the refactor/better_mocks branch 2 times, most recently from 4db72b1 to 3a2d694 Compare October 4, 2023 10:16
Copy link
Contributor

@gmulhearn gmulhearn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, just the comments about commented out CI + commented out code (nvm, i missed the description, my bad)

gmulhearn
gmulhearn previously approved these changes Oct 4, 2023
Copy link
Contributor

@gmulhearn gmulhearn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems reasonable to me! although libvcx/node is not really my domain, so approval from Patrik or Miroslav as well would be ideal

Patrik-Stas
Patrik-Stas previously approved these changes Oct 4, 2023
@Patrik-Stas
Copy link
Contributor

Patrik-Stas commented Oct 4, 2023

I stopped commenting eventually, but feel free to delete the commented tests

Very happy about this PR

@bobozaur
Copy link
Contributor Author

bobozaur commented Oct 4, 2023

I stopped commenting eventually, but feel free to delete the commented tests

Very happy about this PR

Alright. Will actually do that, since all the comments are kinda bloaty.

Patrik-Stas
Patrik-Stas previously approved these changes Oct 4, 2023
Signed-off-by: Bogdan Mircea <mirceapetrebogdan@gmail.com>
Signed-off-by: Bogdan Mircea <mirceapetrebogdan@gmail.com>
Signed-off-by: Bogdan Mircea <mirceapetrebogdan@gmail.com>
mirgee
mirgee previously approved these changes Oct 4, 2023
Signed-off-by: Bogdan Mircea <mirceapetrebogdan@gmail.com>
Signed-off-by: Bogdan Mircea <mirceapetrebogdan@gmail.com>
Signed-off-by: Bogdan Mircea <mirceapetrebogdan@gmail.com>
Signed-off-by: Bogdan Mircea <mirceapetrebogdan@gmail.com>
Signed-off-by: Bogdan Mircea <mirceapetrebogdan@gmail.com>
Signed-off-by: Bogdan Mircea <mirceapetrebogdan@gmail.com>
Signed-off-by: Bogdan Mircea <mirceapetrebogdan@gmail.com>
Signed-off-by: Bogdan Mircea <mirceapetrebogdan@gmail.com>
Signed-off-by: Bogdan Mircea <mirceapetrebogdan@gmail.com>
Signed-off-by: Bogdan Mircea <mirceapetrebogdan@gmail.com>
Signed-off-by: Bogdan Mircea <mirceapetrebogdan@gmail.com>
Signed-off-by: Bogdan Mircea <mirceapetrebogdan@gmail.com>
Signed-off-by: Bogdan Mircea <mirceapetrebogdan@gmail.com>
Signed-off-by: Bogdan Mircea <mirceapetrebogdan@gmail.com>
@bobozaur bobozaur dismissed stale reviews from mirgee and Patrik-Stas via 1fddbde October 4, 2023 13:39
@bobozaur bobozaur merged commit 59938d7 into main Oct 4, 2023
38 checks passed
@bobozaur bobozaur deleted the refactor/better_mocks branch October 4, 2023 14:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants