-
Notifications
You must be signed in to change notification settings - Fork 136
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
Integration tests cleanup #1297
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1297 +/- ##
=======================================
Coverage 19.29% 19.29%
=======================================
Files 164 164
Lines 10852 10852
=======================================
Hits 2094 2094
Misses 8758 8758
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Bencher Report
🚨 2 Alerts
Click to view all benchmark results
|
Bencher Report
Click to view all benchmark results
|
Bencher Report
Click to view all benchmark results
|
39e67ac
to
1f80f31
Compare
@@ -343,13 +344,14 @@ pub async fn start_jdc( | |||
std::time::Duration::from_secs(cert_validity_sec), | |||
); | |||
let ret = jd_client::JobDeclaratorClient::new(jd_client_proxy); | |||
tokio::spawn(async move { ret.start().await }); | |||
let ret_clone = ret.clone(); |
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 ret can be changed to job_declarator_client
@@ -54,6 +54,7 @@ pub static IS_NEW_TEMPLATE_HANDLED: AtomicBool = AtomicBool::new(true); | |||
/// switching to backup Pools in case of declared custom jobs refused by JDS (which is Pool side). | |||
/// As a solution of last-resort, it is able to switch to Solo Mining until new safe Pools appear | |||
/// in the market. | |||
#[derive(Debug, Clone)] |
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 don’t understand the need to make the configuration structure clonable. Why are we returning configuration objects downstream? While this approach does make all our starter APIs consistent, I don’t see any other benefit. Am I missing something?
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.
You are right that none of them is currently used beside the Sniffer
. I think in the future some of those roles will have some functionality that we might need while testing, thats the main motivation for including them
functions return signature
1f80f31
to
0a81c0e
Compare
@jbesraa please see these commits and if they make sense, cherry-pick them into this PR: with #1317 in mind, I forked this branch and added the following commits:
if we merge this PR including these commits we can close #1317 the following commit is a simple cleanup of confusing variable naming: |
Resolves #1296 #1244 #1181 #1234