Replace usages of ::std::env::temp_dir with tempdir crate in tests #6215
Conversation
|
It looks like @nicolasochem signed our Contributor License Agreement. 👍 Many thanks, Parity Technologies CLA Bot |
| assert_eq!(ethash.cache.lock().recent_epoch.unwrap(), 2); | ||
| assert_eq!(ethash.cache.lock().prev_epoch.unwrap(), 0); | ||
| #[cfg(test)] | ||
| mod test { |
There was a problem hiding this comment.
convention would call this "mod tests", here and other places
| } | ||
| #[cfg(test)] | ||
| mod test { | ||
| extern crate tempdir; |
There was a problem hiding this comment.
@paritytech/core-devs any opinion on including the crate within the module? I'd personally prefer it in the crate root.
There was a problem hiding this comment.
I agree, but only if it's used in more than 1 place by this crate
//ethash/src/lib.rs
#[cfg(test)]
extern crate tempdir;There was a problem hiding this comment.
yep, i was leaning that direction as well. although as Rust will (in a few versions) infer extern crate declarations, I thought it might be best to leave them as easy to hunt down as possible
There was a problem hiding this comment.
really? is there an rfc for that?
There was a problem hiding this comment.
Yes, there is. I personally hope it doesn't happen, although that might just be status quo bias talking.
It's not a certainty though, the PR is yet to be merged. We have this pattern all over the codebase already:
➤ rg '#\[cfg' -A 2 | rg '\bmod\b' -A 1 | rg 'extern crate' | wc -l
10
rphmeier
added
A5-grumble 🔥
| fn test_get_cache_size() { | ||
| // https://github.com/ethereum/wiki/wiki/Ethash/ef6b93f9596746a088ea95d01ca2778be43ae68f#data-sizes | ||
| assert_eq!(16776896usize, get_cache_size(0)); | ||
| assert_eq!(16776896usize, get_cache_size(1)); |
| let boundary = [0x00, 0x00, 0x00, 0x00, 0x00, 0x01, 0x3e, 0x9b, 0x6c, 0x69, 0xbc, 0x2c, 0xe2, 0xa2, 0x4a, 0x8e, 0x95, 0x69, 0xef, 0xc7, 0xd7, 0x1b, 0x33, 0x35, 0xdf, 0x36, 0x8c, 0x9a, 0xe9, 0x7e, 0x53, 0x84]; | ||
| let nonce = 0xd7b3ac70a301a249; | ||
| // difficulty = 0x085657254bd9u64; | ||
| let light = Light::new(TempDir::new("").unwrap(), 486382); |
There was a problem hiding this comment.
I don't think it will really work, it's dropped right after start so if the test passes it means that it doesn't really use the directory anyway or it creates it and never removes in which case it doesn't make it any better than ::std::env::temp_dir()
(here and other occurrences)
| let mut dir = env::temp_dir(); | ||
| dir.push(format!("{:x}-{:x}", rng.next_u64(), rng.next_u64())); | ||
| dir | ||
| let temp_path = TempDir::new("").unwrap(); |
There was a problem hiding this comment.
- Identation
- The directory is not really removed after the test so don't see any point in using the crate here (in the way it's currently used)
There was a problem hiding this comment.
This module can be removed entirely, cause it duplicates TempDir logic
|
Any progress on that one? |
|
stale. closing. |
Fix #6147