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

Eradicate mediated connection from issuance and presentation integration tests, part 1 #967

Merged
merged 34 commits into from
Sep 8, 2023

Conversation

mirgee
Copy link
Contributor

@mirgee mirgee commented Sep 6, 2023

Reduces the number of issuance and presentation integration tests relying on mediated connection to exchange messages instead of exchanging them directly between the SM handlers, with the goal of removing their dependency on agency client and MediatedConnection and eventually making the integration tests runnable without a mediator.

This change results in a minor test speedup, however the main bottleneck is schema / cred. def. creation, which can be mediated by sharing schemas / credential definitions between tests wherever possible.

@mirgee mirgee changed the title Refactor/integration tests Eradicate mediated connection from issuance and presentation integration tests, part 1 Sep 6, 2023
@codecov-commenter
Copy link

codecov-commenter commented Sep 6, 2023

Codecov Report

Merging #967 (9cb8cd5) into main (c08650a) will decrease coverage by 4.19%.
Report is 2 commits behind head on main.
The diff coverage is 71.61%.

@@            Coverage Diff             @@
##             main     #967      +/-   ##
==========================================
- Coverage   39.32%   35.13%   -4.19%     
==========================================
  Files         414      413       -1     
  Lines       28863    27471    -1392     
  Branches     6183     5811     -372     
==========================================
- Hits        11349     9653    -1696     
- Misses      14175    14776     +601     
+ Partials     3339     3042     -297     
Flag Coverage Δ
unittests-aries-vcx 35.13% <71.61%> (-4.19%) ⬇️

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

Files Changed Coverage Δ
aries_vcx/src/common/test_utils.rs 94.22% <ø> (+0.67%) ⬆️
aries_vcx/src/handlers/issuance/holder.rs 42.67% <ø> (-17.33%) ⬇️
aries_vcx/src/handlers/out_of_band/receiver.rs 25.16% <0.00%> (ø)
aries_vcx/src/handlers/util.rs 12.94% <ø> (-4.71%) ⬇️
...ation/verifier/states/presentation_request_sent.rs 55.26% <0.00%> (ø)
aries_vcx/src/utils/devsetup.rs 43.35% <ø> (-4.20%) ⬇️
aries_vcx/tests/test_agency.rs 77.60% <ø> (ø)
aries_vcx/tests/test_pool.rs 83.82% <ø> (ø)
aries_vcx/tests/utils/devsetup_alice.rs 86.44% <ø> (+13.80%) ⬆️
aries_vcx/tests/utils/devsetup_faber.rs 85.29% <ø> (+4.62%) ⬆️
... and 7 more

... and 54 files with indirect coverage changes

@mirgee mirgee marked this pull request as ready for review September 6, 2023 13:19
Patrik-Stas
Patrik-Stas previously approved these changes Sep 6, 2023
Copy link
Contributor

@Patrik-Stas Patrik-Stas left a comment

Choose a reason for hiding this comment

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

this is amazing 🚀 Significant improvement in readability and mental overhead reading the tests, now that connections are gone.

Left some comments but generally ✅

aries_vcx/tests/test_connection.rs Outdated Show resolved Hide resolved
aries_vcx/tests/test_creds_proofs.rs Show resolved Hide resolved
aries_vcx/tests/test_creds_proofs.rs Outdated Show resolved Hide resolved
aries_vcx/tests/utils/scenarios.rs Outdated Show resolved Hide resolved
Signed-off-by: Miroslav Kovar <miroslav.kovar@absa.africa>
Signed-off-by: Miroslav Kovar <miroslav.kovar@absa.africa>
Signed-off-by: Miroslav Kovar <miroslav.kovar@absa.africa>
Signed-off-by: Miroslav Kovar <miroslav.kovar@absa.africa>
Signed-off-by: Miroslav Kovar <miroslav.kovar@absa.africa>
Signed-off-by: Miroslav Kovar <miroslav.kovar@absa.africa>
Signed-off-by: Miroslav Kovar <miroslav.kovar@absa.africa>
Signed-off-by: Miroslav Kovar <miroslav.kovar@absa.africa>
Signed-off-by: Miroslav Kovar <miroslav.kovar@absa.africa>
…nection

Signed-off-by: Miroslav Kovar <miroslav.kovar@absa.africa>
Signed-off-by: Miroslav Kovar <miroslav.kovar@absa.africa>
Signed-off-by: Miroslav Kovar <miroslav.kovar@absa.africa>
…tion

Signed-off-by: Miroslav Kovar <miroslav.kovar@absa.africa>
Signed-off-by: Miroslav Kovar <miroslav.kovar@absa.africa>
Signed-off-by: Miroslav Kovar <miroslav.kovar@absa.africa>
Signed-off-by: Miroslav Kovar <miroslav.kovar@absa.africa>
…ncy_pool_real_proof)

Signed-off-by: Miroslav Kovar <miroslav.kovar@absa.africa>
Signed-off-by: Miroslav Kovar <miroslav.kovar@absa.africa>
…l_to_select_credentials_for_predicate

Signed-off-by: Miroslav Kovar <miroslav.kovar@absa.africa>
Signed-off-by: Miroslav Kovar <miroslav.kovar@absa.africa>
Signed-off-by: Miroslav Kovar <miroslav.kovar@absa.africa>
Signed-off-by: Miroslav Kovar <miroslav.kovar@absa.africa>
Signed-off-by: Miroslav Kovar <miroslav.kovar@absa.africa>
Signed-off-by: Miroslav Kovar <miroslav.kovar@absa.africa>
Signed-off-by: Miroslav Kovar <miroslav.kovar@absa.africa>
Signed-off-by: Miroslav Kovar <miroslav.kovar@absa.africa>
Signed-off-by: Miroslav Kovar <miroslav.kovar@absa.africa>
Signed-off-by: Miroslav Kovar <miroslav.kovar@absa.africa>
Signed-off-by: Miroslav Kovar <miroslav.kovar@absa.africa>
Signed-off-by: Miroslav Kovar <miroslav.kovar@absa.africa>
@mirgee mirgee changed the base branch from main to hotfix/vdrproxy-genesis September 7, 2023 08:13
Signed-off-by: Miroslav Kovar <miroslav.kovar@absa.africa>
Base automatically changed from hotfix/vdrproxy-genesis to main September 7, 2023 09:43
Signed-off-by: Miroslav Kovar <miroslav.kovar@absa.africa>
Signed-off-by: Miroslav Kovar <miroslav.kovar@absa.africa>
Signed-off-by: Miroslav Kovar <miroslav.kovar@absa.africa>
@Patrik-Stas
Copy link
Contributor

Patrik-Stas commented Sep 7, 2023

Goddamn Jimmy, this is some serious gourmet shiiiit!
image

I am even looking at integration-tests-cntd branch which seems to be trimmed down even a lot more than this. I might be getting ahead of myself, because I don't know what's your working strategy, but noticed on that branch, mysql tests are also removed - I'd keep that; do you want to remove them?

One more intermediate remark, and again maybe you already plan doing something like this, just sharing my current thoughts - relating to our private discussion yesterday about scenario.rs:

  • I would suggest to move methods which related to a single agent as an Agent's impl method (and ideally merge Alice & Faber into one singular Agent abstraction); such as
pub async fn accept_proof_proposal(
    faber: &mut Faber,
    verifier: &mut Verifier,
    presentation_proposal: AriesMessage,
) -> AriesMessage {
  • While keeping in workflows methods which contain 2-agent interaction, such as
pub async fn _exchange_credential(
    consumer: &mut Alice,
    institution: &mut Faber,
    credential_data: String,
    cred_def: &CredentialDef,
    rev_reg: &RevocationRegistry,
    comment: Option<&str>,
) -> Issuer {

@mirgee
Copy link
Contributor Author

mirgee commented Sep 8, 2023

I am even looking at integration-tests-cntd branch which seems to be trimmed down even a lot more than this. I might be getting ahead of myself, because I don't know what's your working strategy, but noticed on that branch, mysql tests are also removed - I'd keep that; do you want to remove them?

No worries, mysql tests will be preserved to the extent they are useful, as discussed :)

  • I would suggest to move methods which related to a single agent as an Agent's impl method (and ideally merge Alice & Faber into one singular Agent abstraction); such as ...

I totally agree that this is the desired final state, pruning the current Alice and Faber just an intermediate step. They will next be merged into a unified test agent struct, followed by implementation of single-agent-methods and refactoring of currently so-called "scenarios".

Another (probably less favorable) option would be to get rid of test agent representation altogether for now (perhaps keeping it just as a data structure holding profile equivalents and whatever handlers necessary for the test at hand), awaiting a "more final", generic agent implementation (i.e. useful outside of tests as well).

@Patrik-Stas Patrik-Stas merged commit 3b77552 into main Sep 8, 2023
30 checks passed
@Patrik-Stas Patrik-Stas deleted the refactor/integration-tests branch September 8, 2023 15:56
bobozaur pushed a commit that referenced this pull request Sep 14, 2023
…ion tests, part 1 (#967)

Refactor testing - eradicate mediated connection from issuance and presentation integration tests, part 1 (#967)

Signed-off-by: Miroslav Kovar <miroslav.kovar@absa.africa>
Signed-off-by: Bogdan Mircea <mirceapetrebogdan@gmail.com>
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.

3 participants