GH-611: Accountant Overlapping Scans #176
Conversation
…e scan is already running
…h an empty vector
node/src/accountant/mod.rs
Outdated
| scanners: Scanners::default(), | ||
| financial_statistics: FinancialStatistics::default(), | ||
| report_accounts_payable_sub: None, | ||
| scanners: Scanners::new( |
There was a problem hiding this comment.
This is really a detail but I'd think longer of moving the Scanners::new() constructor before the Accountant fields assignment, manipulating just one variable having the Scanners. Again, for code symmetricity.
There was a problem hiding this comment.
Correct me if I'm wrong.
The expectation of this comment is to move the field scanners to the top while initialising Neighborhood?
If yes, you may mark this comment as resolved as I modified the code according to the above sentence. 😉
There was a problem hiding this comment.
Oh, no. I actually don't understand YOUR interpretation. I can clarify my original suggestion though.
I didn't want nothing more than you assign the output of the Scanners::new(...) constructor to a variable first and then you assign the variable to the field within Accountant{...}. I think it will look nicer like so because all the fields will have just a simple expressions next to them, not a big block of code as in the case of this constructor.
| System::current().stop(); | ||
| system.run(); | ||
| let blockchain_bridge_recording = blockchain_bridge_recording_arc.lock().unwrap(); | ||
| TestLogHandler::new().exists_log_containing(&format!( |
There was a problem hiding this comment.
It's a custom that we put the log assertions at the furthest end of the test.
| let notify_later_receivable_params = notify_later_receivable_params_arc.lock().unwrap(); | ||
| assert_eq!(begin_scan_params.len(), 2); |
There was a problem hiding this comment.
This assertion is too vague. Unit types param assertions with () are acceptable only if no other option is left. So is asserting on the count of params with len().
Modify the mock so that it captures as much information from the params as possible. I went over to see what's there and you can at least capture the timestamp, plus you will also be able to capture the response skeleton, both should be done here.
It's actually quite important to check the rightness of the timestamps, with grounding two limiting timestamps in between which you should find the actual one. You have to prove that the SystemTime::new() calls produce the right values at the right place. Also, mentioning the params will help the reader to get a picture in their mind of what the method begun_scan() is supposed to do.
node/src/node_configurator/unprivileged_parse_args_configuration.rs
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| impl PayableDaoFactory for PayableDaoFactoryMock { | ||
| fn make(&self) -> Box<dyn PayableDao> { | ||
| *self.called.borrow_mut() = true; | ||
| Box::new(self.mock.borrow_mut().remove(0)) | ||
| if self.make_results.borrow().len() == 0 { |
There was a problem hiding this comment.
Ehhhmm....we've never done like so with the existing mocks (and there are a lots of them) and that's I don't like this change in the way of treating it. I sort of understand why you're doing that but still I'd advice to think twice if you mean this is so important. If not, I'm okay if it goes away.
We normally navigate ourselves by the stack trace which is a surprisingly good and reliable method telling you exactly where it blew up if the vector of results is already empty at the moment.
| time: SystemTime, | ||
| account: &ReceivableAccount, | ||
| ) -> (String, Duration) { | ||
| let balance = format!("{}", (account.balance as f64) / 1_000_000_000.0); |
There was a problem hiding this comment.
Please remove this division by one billion ( 10^9 ?).
We should show it more accurately than we're doing here. We can display Wei. No big deal.
| fn payable_with_low_payout_threshold_is_marked_unqualified() { | ||
| let now = SystemTime::now(); | ||
| let payment_thresholds = PaymentThresholds::default(); | ||
| let debt = payment_thresholds.permanent_debt_allowed_gwei + 1; |
There was a problem hiding this comment.
You should try to get as close to the threshold as possible, not to be almost as far as possible, the opposite...
Try active computing of a triangle to get the slope of the declining line and then to compute a point somewhere on the threshold (ideally near to its middle) and then just subtract a few units (maybe just 1, 2, or 3...) to get a slightly smaller value. It will be as proofing as we can ever do.
* GH-611: refactor the Accountant's constructor * GH-611: remove contract test for eth ropsten * GH-611: rename function names that handles scan requests * GH-611: rename message to scan_message * GH-611: remove the eprintln!() from the production code * GH-611: use response_skeleton to geneate logs in different severity * GH-611: remove code duplication in the handling of scan requests * GH-611: simplify the financial_statistics * GH-611: refactor test scan_request_from_ui_is_handled_in_case_the_scan_is_already_running() * GH-611: Add the ability to log complete tx hash * GH-611: refactor test accountant_receives_new_payments_to_the_receivables_dao() * GH-611: refactor accountant_scans_after_startup() * GH-611: minor test improments related to duration and begin_scan_params * GH-611: strengthen the test start_message_triggers_no_scans_in_suppress_mode() * GH-611: minor improvements in tests and renames * GH-611: remove duration from the test accountant_does_not_initiate_another_scan_in_case_it_receives_the_message_and_the_scanner_is_running() * GH-611: log full hash * GH-611: remove the contract test for ropsten * GH-611: rename make_scan_intervals_with_defaults() to default_scan_intervals() * GH-611: use a default implementation for the default scan intervals * GH-611: stop using a mutable reference of BootstrapperConfig to build Accountant * GH-611: rename test in blockchain_bridge.rs * GH-611: finish review changes for scanners.rs * GH-611: change the log when receivable scanner no new payments from the blockchain bridge * GH-611: change the error message when the scan_finished() is called but the timestamp is not found. * GH-611: working on refactoring the mark_as_ended() * GH-611: write a common function for updating the timestamp when a scan is ended. * GH-611: rename function names and add function names in the panic messages for the NullScanner * GH-611: strengthen the test scanners_struct_can_be_constructed_with_the_respective_scanners() * GH-611: improve test payable_scanner_can_initiate_a_scan() * GH-611: minor changes in the scanners.rs * GH-611: change the way we log NothingToProcess error * GH-611: strengthen the test receivable_scanner_scans_for_delinquencies() * GH-611: improve test for handle_none_status() * GH-611: remove the pub keyword from multiple functions inside the impl block of BeginScanError * GH-611: review changes for scanners_tools.rs * GH-611: use builder approach to build the scanners for tests * GH-611: remove the wrapper of migrator_config (risky) * GH-611: remove the wrapper from the when_pending_too_long (risky) * GH-611: put the mistakenly removed contract test back * GH-611: remove some unnecessary comments * GH-611: make the suppress_initial_scans flag just a boolean rather than wrapping * GH-611: minor remaining code changes * GH-611: Review 2 (#217) * GH-611: rename the function names again * GH-611: rename the field to when_pending_too_long_sec * GH-611: refactor test scan_request_from_ui_is_handled_in_case_the_scan_is_already_running * GH-611: use use_logs_containing inside the test periodical_scanning_for_receivables_and_delinquencies_works * GH-611: change the scan intervals back to their unique values * GH-611: improve timestamp_as_string function in scanners.rs * GH-611: rename the function to remove_timestamp_and_log * GH-611: migrate remove_timestamp_and_log to ScannerCommon * GH-611: change to exists_log_containing * GH-611: refactor handle_error() and remove log() inside BeginScanError * GH-611: use macro to remove code duplication in scanners.rs * GH-611: remove unnecessary modifications * GH-611: use the best practices of builder pattern for the individual scanner mocks * GH-611: refactor the constructor of Accountant * GH-611: rename the tests in scanners_tools.rs * GH-611: remove take() from the constructor of Accountant * GH-611: use just borrow for financial statistics * GH-611: remove the test that was testing panic * GH-611: remove assertions from the test start_message_triggers_no_scans_in_suppress_mode * GH 611: Review 3 (#220) * GH-611: remove the clone from the earning wallet iniside the Accountant's constructor * GH-611: rename the function to simply remove_timestamp() * GH-611: remove clone from the payment_thresholds * GH-611: change the test_name variable in tests for remove_timestamp
…mains to be suspicious and will need to be examined for duplication or their utility.
…o file names around); also refactoring investigate_debt_extremes
…r injecting individual mock DAOs
… that completed from my part
…error_is_handled_properly
No description provided.