On 8/16/2023 11:35 PM, Eric Blake wrote:
A couple of integration tests are added in rust/tests. They are mostly ported from the OCaml tests. --- Overall, this looked like a nice counterpart to the OCaml unit tests, and I was able to easily figure out how to amend my own tests in for
On Thu, Aug 03, 2023 at 03:36:06PM +0000, Tage Johansson wrote: the unit tests I added in my 64-bit extension work. Keeping similar unit test numbers across language bindings has been a big boon :-)
Style question:
+++ b/rust/tests/test_250_opt_set_meta.rs + +/// A struct with information about set meta contexts. +#[derive(Debug, Clone, PartialEq, Eq)] +struct CtxInfo { + /// Whether the meta context "base:allocation" is set. + has_alloc: bool, + /// The number of set meta contexts. + count: u32, +} Here, you use a trailing comma,
+ +fn set_meta_ctxs(nbd: &libnbd::Handle) -> libnbd::Result<CtxInfo> { + let info = Arc::new(Mutex::new(CtxInfo { + has_alloc: false, + count: 0, + })); + let info_clone = info.clone(); + let replies = nbd.opt_set_meta_context(move |ctx| { + let mut info = info_clone.lock().unwrap(); + info.count += 1; + if ctx == CONTEXT_BASE_ALLOCATION { + info.has_alloc = true; + } + 0 + })?; + let info = Arc::into_inner(info).unwrap().into_inner().unwrap(); + assert_eq!(info.count, replies); + Ok(info) +} + ... + + // nbdkit does not match wildcard for SET, even though it does for LIST + nbd.clear_meta_contexts().unwrap(); + nbd.add_meta_context("base:").unwrap(); + assert_eq!( + set_meta_ctxs(&nbd).unwrap(), + CtxInfo { + count: 0, + has_alloc: false + } whereas here, you did not. Does it make a difference? 'make check' still passes, and rustfmt doesn't complain, if I temporarily add a trailing comma here.
No, it doesn't make a difference. I nearly always runs rustfmt on my code but it doesn't seem that rustfmt cares about that comma.
I also note that 'rustfmt --check rust/tests/*.rs' flags a few style suggestions. I had started work on automating gofmt for all checked-in *.go files; maybe I should revive that patch and extend it to also automate rustfmt on all checked-in *.rs files. Here's what rustfmt suggested to me (rustfmt 1.5.2-stable ( ) on Fedora 38):
Diff in /home/eblake/libnbd/rust/tests/test_200_connect_command.rs at line 17:
#![deny(warnings)]
- #[test] fn test_connect_command() { let nbd = libnbd::Handle::new().unwrap(); Diff in /home/eblake/libnbd/rust/tests/test_200_connect_command.rs at line 24: - nbd.connect_command(&[ - "nbdkit", - "-s", - "--exit-with-parent", - "-v", - "null", - ]) - .unwrap(); + nbd.connect_command(&["nbdkit", "-s", "--exit-with-parent", "-v", "null"]) + .unwrap(); }
Diff in /home/eblake/libnbd/rust/tests/test_300_get_size.rs at line 17:
#![deny(warnings)]
- #[test] fn test_get_size() { let nbd = libnbd::Handle::new().unwrap(); Diff in /home/eblake/libnbd/rust/tests/test_620_stats.rs at line 17:
#![deny(warnings)]
- #[test] fn test_stats() { let nbd = libnbd::Handle::new().unwrap(); Diff in /home/eblake/libnbd/rust/tests/test_620_stats.rs at line 31: // Connection performs handshaking, which increments stats. // The number of bytes/chunks here may grow over time as more features get // automatically negotiated, so merely check that they are non-zero. - nbd.connect_command(&[ - "nbdkit", - "-s", - "--exit-with-parent", - "-v", - "null", - ]) - .unwrap(); + nbd.connect_command(&["nbdkit", "-s", "--exit-with-parent", "-v", "null"]) + .unwrap();
let bs1 = nbd.stats_bytes_sent(); let cs1 = nbd.stats_chunks_sent();
Automating rustfmt on all checked-in rust files sounds like a good idea. It is strange that those file were not proprly formatted since I just mentioned that I nearly always runs rustfmt on every edit. For the files which are not upstream yet, (test_async_*), I will fix the formatting in the next patch series. For the other files I will create an extra patch to correct the formatting. -- Best regards, Tage