-
Notifications
You must be signed in to change notification settings - Fork 27
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
feat(p2p): remove sync-v1 code and disable tests #1197
base: master
Are you sure you want to change the base?
Conversation
Bencher Report
Click to view all benchmark results
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1197 +/- ##
==========================================
- Coverage 84.93% 83.11% -1.82%
==========================================
Files 323 320 -3
Lines 25124 24537 -587
Branches 3859 3762 -97
==========================================
- Hits 21340 20395 -945
- Misses 3041 3394 +353
- Partials 743 748 +5 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These changes seem unrelated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True, these changes aren't needed.
self.assertFalse(conn.tr1.disconnecting) | ||
self.assertFalse(conn.tr2.disconnecting) | ||
|
||
def test_update_whitelist(self) -> None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test seems valid for sync v2. Why did you remove it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed them because they were running only on sync-v1, but I should've changed it to sync-v2, with some refactoring.
class _SerializationV2OnlyTest(unittest.TestCase): | ||
__test__ = False | ||
|
||
def test_serialization_tips(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test seems to not be sync-v1 specific. Why did you remove it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a bad diff, the test wasn't removed. I moved the sync-v2-only tests to the main test class and removed the sync-v1-only tests, the diff didn't show this well.
Motivation
We've had sync-v1 in unsafe mode for a while and bridge nodes are on their way out, also all supported releases have sync-v2 on by default, and as far as we know there's no one using sync-v1 anymore.
Acceptance Criteria
Checklist
master
, confirm this code is production-ready and can be included in future releases as soon as it gets merged