refactor(test): Extract common crypto mock server helper #5005
Conversation
BillCarsonFr
changed the base branch from
main
to
valere/crypto/to_device_encrypt_helper
| pub struct MatrixKeysMockServer { | ||
| /// The underlying [`wiremock`] [`MockServer`] | ||
| pub server: MockServer, | ||
| keys: Arc<Mutex<Keys>>, | ||
| token_to_user_id_map: Arc<Mutex<BTreeMap<String, OwnedUserId>>>, | ||
| token_counter: AtomicU32, | ||
| } |
There was a problem hiding this comment.
This doesn't really incorporate the Keys struct into MatrixMockServer instead it creates another mock server concept with much less functionality.
I don't think we'd want to have two of those.
There was a problem hiding this comment.
Got it, I did refactor it to extend the existing MockServer 9766ca1
I kept the crypto API mocking in the new file for code organisation, Is it ok?
I did a second commit to take benefit of the refactoring and re-use a functionnality already in the base mock server instead of using another custom method fcb9070
BillCarsonFr
force-pushed
the
valere/crypto/extract_crypto_mockserver_helper
branch
from
50829d3 to
cf13d30
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5005 +/- ##
==========================================
- Coverage 85.82% 85.76% -0.06%
==========================================
Files 325 326 +1
Lines 35984 36183 +199
==========================================
+ Hits 30883 31034 +151
- Misses 5101 5149 +48 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
BillCarsonFr
requested review from
andybalaam and
poljar
and removed request for
a team
There was a problem hiding this comment.
I would have preferred some doc examples on the individual methods instead on the impl block which won't be shown anywhere besides in the source as far as I know.
Some renaming wouldn't have hurt either. I think we can address that in separate PRs so we don't block your stack of PRs on this for too long.
| request.event_type.clone(), | ||
| request.txn_id.clone(), | ||
| request.messages.clone(), | ||
| ); | ||
|
|
||
| let send_result = self | ||
| .client | ||
| .send_inner(request, Some(RequestConfig::short_retry()), Default::default()) | ||
| .send_inner(ruma_request, Some(RequestConfig::short_retry()), Default::default()) |
There was a problem hiding this comment.
Was this added by accident? It's not really topical to the PR.
| /// Extends the `MatrixMockServer` with useful methods to help mocking | ||
| /// matrix crypto API and perform integration test with encryption. | ||
| /// | ||
| /// It implements mock endpoints for the `keys/upload`, will store the uploaded | ||
| /// devices and serves them back for incoming `keys/query`. It is also storing | ||
| /// and claiming one-time-keys, allowing to set up working olm sessions. | ||
| /// | ||
| /// Adds some helpers like `exhaust_one_time_keys` that allows to simulate a | ||
| /// client running out of otks. More can be added if needed later. | ||
| /// | ||
| /// It works like this: | ||
| /// * Start by creating the mock server like this [`MatrixMockServer::new`]. | ||
| /// * Then mock the crypto API endpoints | ||
| /// [`MatrixMockServer::mock_crypto_endpoints_preset`]. | ||
| /// * Create your test client using | ||
| /// [`MatrixMockServer::client_builder_for_crypto_end_to_end`], this is | ||
| /// important as it will set up an access token that will allow to know what | ||
| /// client is doing what request. | ||
| /// | ||
| /// The [`MatrixMockServer::set_up_alice_and_bob_for_encryption`] will set up | ||
| /// two olm machines aware of each other and ready to communicate. |
There was a problem hiding this comment.
That won't get rendered anywhere, right?
Draft because it is a follow up of #4998
Extracted a helper to create a wiremock server that allows to perform full end to end encryption tests.
Note: In the existing verification test there was a
known_deviceslist, but this was not used (the test work fine if you remove it), so I removed it when extracting the mock helpersSigned-off-by: