cleanup clippy tests #10172
cleanup clippy tests #10172
Conversation
|
Strange, |
|
@mvines Well, I thought |
|
Thanks for working on this again!! @svenski123 How about enabling this in this pr?: #10030 (comment) Otherwise, this surely reoccurs. Let's close the gap!:
Also, your concern at #5503 is fixed already. :) from my comment there:
I guessed this will occur. xD Also, you don't need to run all of ci tests locally, just believe your edit and test it on our ci. For these tiny edits this is enough. |
| @@ -895,8 +895,7 @@ mod tests { | |||
| let (subscriber, _id_receiver, receiver) = Subscriber::new_test("voteNotification"); | |||
|
|
|||
| // Setup Subscriptions | |||
| let subscriptions = | |||
| RpcSubscriptions::new(&exit, bank_forks.clone(), block_commitment_cache.clone()); | |||
| let subscriptions = RpcSubscriptions::new(&exit, bank_forks, block_commitment_cache); | |||
There was a problem hiding this comment.
@svenski123 Could you check the flakiness of this test test_vote_subscribe locally? it seems like this test got a lot unstable with removing .clone() on ci:
- https://buildkite.com/solana-labs/solana/builds/24611#4e149382-e000-4a94-85c3-c8f520e42009/467-993
- https://buildkite.com/solana-labs/solana/builds/24611#b6c44f63-29ad-41a5-a030-c16c22e8dc06/128-654
- https://buildkite.com/solana-labs/solana/builds/24611#b16ec14f-7317-477a-a337-a09bacd3a411/133-659
- https://buildkite.com/solana-labs/solana/builds/24611#b38fbc2e-0e10-485c-9133-9109113aedc7/116-642
My guess is that this or that .clone() takes some time and removing that makes it more susceptible to the some kind of race condition. If you're brave enough, feel free to go down the rabbit hole or just add #allow(...)...
FYI, this test has been a bit flaky for some time, iirc.
|
Having removed clippy complaints before, I obviously couldn't reintroduce them again in #9980 (though not using a type alias for tuple-of-hash-of-integer-to-vector-of-struct wasn't my idea :)). So I tugged on that thread and some more stuff fell out - hence this PR. test_vote_subscribe fails locally - seems one of the channels gets closed prematurely, and this is somehow linked to the earlier drop due to one of the missing clones. So this testcase needs to be picked apart a bit, gone through and put together (rather than simply sticking the clone back in). |
hehe ;) I was pretty sure you'll introduce one for it.
I see
Hehe, it seems that you're interested. Thanks! |
|
@svenski123 How is this going? :) Still trying to fix the root cause? Or am I making you to wait for something? You can fix it at your pace, but rahter I want to merge the rest of clippy nice fixes you did and |
|
Hi @ryoqun ! Apologies I've not had much time for development this week so this has been dragging out. Also there's quite a lot going on in that testcase and rust's various async / Future related libraries take some time to work through. I'll try to get to it this evening but if not I'll put the clone back that makes it work with a clippy override to unblock this. |
svenski123
force-pushed
the
clippy-cleanup-001
branch
from
2eee7b2 to
a12ae1c
Compare
Codecov Report
@@ Coverage Diff @@
## master #10172 +/- ##
=========================================
- Coverage 81.3% 81.3% -0.1%
=========================================
Files 288 288
Lines 66906 66915 +9
=========================================
+ Hits 54433 54437 +4
- Misses 12473 12478 +5 |
I see! Please take your time. There should be no rush. :)
Oh I see! I'm also not that knowledgeable with that test, too... Thanks for looking! Also, feel free ask any questions if that's makes it easy for you. For now, let's merge the rest! We always welcome your contribution. :) |
Problem
Clippy complaints in test code have clept in again.
Summary of Changes
Clean up the clippy complaints