Rewrite remote_cache_tests.rs to mock the local runner#11474
Conversation
# Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]
[ci skip-build-wheels]
Thanks Tom and Greg! # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]
# Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]
# Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]
We need this for the cache write tests # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]
# Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]
# Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]
We already have the atomic counter to keep track of local runs. It was adding unnecessary complexity # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]
Eric-Arellano
changed the title
remote_cache_tests.rs to mock the local runnerremote_cache_tests.rs to mock the local runner
Eric-Arellano
left a comment
Eric-Arellano
left a comment
There was a problem hiding this comment.
I don't expect the diff to be very insightful, given how much I rewrote these tests. Probably best to look at this test file as a completely new thing. (Although make_tree_from_directory and below is identical).
Thank you three for your help pair programming on this!!
tdyas
left a comment
tdyas
left a comment
There was a problem hiding this comment.
lgtm plus some minor comments
| #[derive(Clone)] | ||
| struct MockLocalCommandRunner { | ||
| result: Result<FallibleProcessResultWithPlatform, String>, | ||
| call_counter: Arc<Mutex<u32>>, |
There was a problem hiding this comment.
More idiomatic is to use an atomic counter type like AtomicUsize.
There was a problem hiding this comment.
More idiomatic is to use an atomic counter type like AtomicUsize.
I'm curious why you think it's more idiomatic to use an atomic type. My understanding is that the atomic types use some platform-specific overhead compared to the standard integer types, in order to provide thread safety, which we don't need in a test. So using an atomic type would add overhead for no particular gain here.
There was a problem hiding this comment.
Arc already has that same overhead give or take to implement its capital A IIUC. If threads aren't in play in tests the proof is dropping the Arc to an Rc and seeing if it compiles.
There was a problem hiding this comment.
I'm curious why you think it's more idiomatic to use an atomic type.
Just my impression that it is always nice to avoid a lock if possible, and atomic types let one do just that. This is test code so I'm not too concerned if this remains a Mutex for readability or other purposes.
There was a problem hiding this comment.
Rc breaks because we store this on CommandRunner, which must implement Send.
I'll go with AtomicUsize instead of Mutex<u32> for clearer semantics.
| // NB: We bundle these into a struct to ensure they share the same lifetime. | ||
| struct StoreSetup { | ||
| pub store: Store, | ||
| pub store_dir: PathBuf, | ||
| pub cas: StubCAS, | ||
| } |
| 10 * 1024 * 1024, | ||
| Duration::from_secs(1), | ||
| BackoffConfig::new(Duration::from_millis(10), 1.0, Duration::from_millis(10)).unwrap(), |
There was a problem hiding this comment.
remote_tests.rs has OVERALL_DEADLINE_SECS and RETRY_INTERVAL constants for some of these values. Maybe do something similar in this file?
There was a problem hiding this comment.
As in, share the constants across files? Or factor these up into constants? Imo the constants wouldn't be worth it very much because they're not used elsewhere.
There was a problem hiding this comment.
Share across files if you think it is worth it? My thought was DRY for test constants.
There was a problem hiding this comment.
Indeed, it looks like the code to create a remote store is very similar across the two files. I think it could be worth factoring up, but probably as a second PR. This diff is already bad enough.
# Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]
4 participants
The tests were originally closer to integration tests, whereas we want high-quality, focused unit tests for both cache reads and cache writes.
This will allow us to do further mocking for the sake of testing speculation in #11429.