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

crypto: remove the fake crypto store test for olm session wedging #3342

Merged
merged 1 commit into from
May 13, 2024

Conversation

bnjbvr
Copy link
Member

@bnjbvr bnjbvr commented Apr 19, 2024

Two reviewers considered that test as not very useful, so I hereby propose to remove it.

@bnjbvr bnjbvr requested a review from a team as a code owner April 19, 2024 08:08
@bnjbvr bnjbvr requested review from poljar and removed request for a team April 19, 2024 08:08
Copy link

codecov bot commented Apr 19, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.95%. Comparing base (83427b3) to head (1d88b9d).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3342      +/-   ##
==========================================
- Coverage   82.96%   82.95%   -0.02%     
==========================================
  Files         245      245              
  Lines       24901    24901              
==========================================
- Hits        20660    20657       -3     
- Misses       4241     4244       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@poljar
Copy link
Contributor

poljar commented Apr 19, 2024

Let's see if we get the complement test ported, the test has some use before that's achieved.

@bnjbvr
Copy link
Member Author

bnjbvr commented Apr 19, 2024

Oh, I thought the other test did test the same effect properly, by adding a session, regenerating the olm machine and checking that the session was then missing from the cache?

@poljar
Copy link
Contributor

poljar commented Apr 19, 2024

Oh, I thought the other test did test the same effect properly, by adding a session, regenerating the olm machine and checking that the session was then missing from the cache?

Well kinda, the other test ensures that the SqliteCryptoStore method does the correct thing, this test ensures that the regenerate_olm() method will call the method to clear the cache.

@bnjbvr
Copy link
Member Author

bnjbvr commented Apr 19, 2024

Ah, so you're saying the SqliteCryptoStore could do the correct thing, without the method to clear the cache getting called? Then agreed, we can merge this later.

@poljar
Copy link
Contributor

poljar commented Apr 19, 2024

Ah, so you're saying the SqliteCryptoStore could do the correct thing, without the method to clear the cache getting called? Then agreed, we can merge this later.

Yeah, we might accidentally remove the call to this method and the dumb test ensures that we don't do so.

@poljar
Copy link
Contributor

poljar commented Apr 29, 2024

This depends on #3343.

@bnjbvr
Copy link
Member Author

bnjbvr commented May 13, 2024

@poljar now that #3343 has been merged, can haz review plz? 🥺

Copy link
Contributor

@poljar poljar left a comment

Choose a reason for hiding this comment

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

Very elegant removal, more of this please.

@bnjbvr bnjbvr force-pushed the bnjbvr/remove-fake-crypto-store-test branch from 317636e to 0ea8f0a Compare May 13, 2024 14:57
@bnjbvr bnjbvr enabled auto-merge (rebase) May 13, 2024 14:57
Two reviewers were suspicious the test wasn't very useful, so I propose
to hereby remove it.
@bnjbvr bnjbvr force-pushed the bnjbvr/remove-fake-crypto-store-test branch from 0ea8f0a to 1d88b9d Compare May 13, 2024 16:16
@bnjbvr bnjbvr merged commit da249bb into main May 13, 2024
35 checks passed
@bnjbvr bnjbvr deleted the bnjbvr/remove-fake-crypto-store-test branch May 13, 2024 16:30
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.

2 participants