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

Add backtrace capturing, prototype a test err handling #1022

Merged
merged 6 commits into from
Nov 6, 2023

Conversation

Patrik-Stas
Copy link
Contributor

@Patrik-Stas Patrik-Stas commented Oct 22, 2023

  • Removed run(fn) test case wraps as they turned out to be no longer necessary. Deleted related dead code.
  • Add feature backtrace_errors; when enabled, backtrace dependency is included and error thrown from aries-vcx contain capture backtrace information
  • Tweak Display impl for AriesVcxError
  • Add Debug impl for AriesVcxError - this is useful when err is thrown from a test run by cargo test runner
  • Modified all integration tests:
    • They return -> Result<(), Box<dyn Error>> instead of -> ().
    • Therefore we can write test code without being coerced into calling unwrap()
    • The only good thing about unwrap() panics was that that they included stacktrace, which is not default with returning an error - but with additions mentioned above, we are not loosing this capability

If agreed, we can continue adopting:

  • same panic-less approach in the rest of the test code
  • backtrace capturing for other sub-crates as well

After rebase

I've temporarily added run2 but I want to question 2 things:

  • What's the purpose of macro run_setup_test(...) as opposed to just calling build_setup_profile().await.run(...)
  • I believe that whole run thing pub async fn run<F>(self, f: impl FnOnce(Self) -> F) is now obsolete, there's no universal setup&cleanup which is valid for every test. In fact, run is not doing anything useful atm, other the running the test itself

@Patrik-Stas Patrik-Stas force-pushed the testing/err-handling branch 2 times, most recently from 72dc7ec to 4750b50 Compare October 22, 2023 21:09
@codecov-commenter
Copy link

codecov-commenter commented Oct 22, 2023

Codecov Report

Merging #1022 (2939007) into main (4e2a5f4) will increase coverage by 0.00%.
The diff coverage is 0.36%.

❗ Current head 2939007 differs from pull request most recent head cbe30ca. Consider uploading reports for the commit cbe30ca to get more accurate results

@@          Coverage Diff           @@
##            main   #1022    +/-   ##
======================================
  Coverage   0.05%   0.05%            
======================================
  Files        384     384            
  Lines      21249   21084   -165     
  Branches    3821    3910    +89     
======================================
  Hits          12      12            
+ Misses     21236   21071   -165     
  Partials       1       1            
Flag Coverage Δ
unittests-aries-vcx 0.05% <0.36%> (+<0.01%) ⬆️

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

Files Coverage Δ
aries_vcx/src/handlers/mediated_connection/mod.rs 0.00% <ø> (ø)
...vcx/src/protocols/issuance/issuer/state_machine.rs 0.00% <ø> (ø)
tools/test_utils/src/devsetup.rs 0.00% <0.00%> (ø)
aries_vcx/tests/test_mysql_wallet.rs 4.00% <0.00%> (+0.15%) ⬆️
aries_vcx/src/errors/error.rs 0.00% <0.00%> (ø)
...es_vcx/src/common/proofs/prover/prover_internal.rs 0.00% <0.00%> (ø)
aries_vcx/tests/test_connection.rs 0.87% <1.66%> (ø)
aries_vcx/tests/test_primitives.rs 1.28% <1.42%> (+0.10%) ⬆️
aries_vcx/tests/test_credentials.rs 1.02% <1.12%> (+0.08%) ⬆️
aries_vcx/tests/test_anoncreds.rs 0.89% <1.01%> (+0.06%) ⬆️
... and 6 more

@bobozaur
Copy link
Contributor

@Patrik-Stas Please note that as part of #1023 I actually copied the test_setup() function from here. I initially wanted to pull some trickery and allow Arc<dyn Trait> in tests but I forgot that the generic arguments in the traits methods make them not trait object safe, so we have to stick with generics. Since I'm moving some things around I figured it could be easier if I copied the implementation there.

@bobozaur bobozaur mentioned this pull request Oct 23, 2023
@Patrik-Stas Patrik-Stas force-pushed the testing/err-handling branch 3 times, most recently from c712666 to c914a37 Compare October 29, 2023 20:19
@bobozaur
Copy link
Contributor

* What's the purpose of macro `run_setup_test(...)` as opposed to just calling `build_setup_profile().await.run(...)`

None, really. Just convenience.

* I believe that whole run thing `pub async fn run<F>(self, f: impl FnOnce(Self) -> F)` is now obsolete, there's no universal setup&cleanup which is valid for every test. In fact, `run` is not doing anything useful atm, other the running the test itself

I assume it could be removed in this case, indeed.

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>
Signed-off-by: Patrik Stas <patrik.stas@absa.africa>
@Patrik-Stas Patrik-Stas requested a review from gmulhearn October 31, 2023 16:40
@Patrik-Stas
Copy link
Contributor Author

This should be ready for re-review.

  • Removed the setup run(fn) wraps as they turned out unnecessary
  • Removed unwrap() from all tests in favor of returning error

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

@gmulhearn gmulhearn merged commit c8b8a6e into main Nov 6, 2023
26 of 27 checks passed
@gmulhearn gmulhearn deleted the testing/err-handling branch November 6, 2023 21:31
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.

4 participants