Re: [PATCH V4 7/8] libgpiod: Add rust tests

[Viresh Kumar <viresh.kumar@linaro.org>]

On 27-07-22, 10:58, Kent Gibson wrote:
> On Fri, Jul 08, 2022 at 05:05:00PM +0530, Viresh Kumar wrote:
> Don't include the test results in the commit message.
> Those are more of a release artifact than a commit artifact.
> It is assumed the tests pass for you or you wouldn't be submitting them.

I wasn't trying to prove that I tested them :)

The idea was to show how module/test names eventually look in the output.

Maybe I could have just replied to this email and pasted it, sure commit message
doesn't need it.

> > diff --git a/bindings/rust/tests/chip.rs b/bindings/rust/tests/chip.rs
> > +    mod configure {
> > +        use super::*;
> > +        const NGPIO: u64 = 16;
> > +        const LABEL: &str = "foobar";
> > +
> > +        #[test]
> > +        fn verify() {
> > +            let sim = Sim::new(Some(NGPIO), Some(LABEL), true).unwrap();
> > +            let chip = Chip::open(sim.dev_path()).unwrap();
> > +
> > +            assert_eq!(chip.get_label().unwrap(), LABEL);
> > +            assert_eq!(chip.get_name().unwrap(), sim.chip_name());
> > +            assert_eq!(chip.get_path().unwrap(), sim.dev_path());
> > +            assert_eq!(chip.get_num_lines(), NGPIO as u32);
> > +            chip.get_fd().unwrap();
> > +        }
> > +
> 
> A test for a basic open on an existing chip and a smoke test of some
> chip methods is called "verify", so chip::configure::verify?

You want me to rename this ? Sorry, got confused :(

Yeah, I am generally bad with naming, suggestions are welcome here :)

> > +        #[test]
> > +        fn line_lookup() {
> > +            let sim = Sim::new(Some(NGPIO), None, false).unwrap();
> > +            sim.set_line_name(0, "zero").unwrap();
> > +            sim.set_line_name(2, "two").unwrap();
> > +            sim.set_line_name(3, "three").unwrap();
> > +            sim.set_line_name(5, "five").unwrap();
> > +            sim.set_line_name(10, "ten").unwrap();
> > +            sim.set_line_name(11, "ten").unwrap();
> > +            sim.enable().unwrap();
> > +
> > +            let chip = Chip::open(sim.dev_path()).unwrap();
> > +
> > +            // Success case
> > +            assert_eq!(chip.find_line("zero").unwrap(), 0);
> > +            assert_eq!(chip.find_line("two").unwrap(), 2);
> > +            assert_eq!(chip.find_line("three").unwrap(), 3);
> > +            assert_eq!(chip.find_line("five").unwrap(), 5);
> > +
> > +            // Success with duplicate names, should return first entry
> > +            assert_eq!(chip.find_line("ten").unwrap(), 10);
> > +
> > +            // Failure
> > +            assert_eq!(
> > +                chip.find_line("nonexistent").unwrap_err(),
> > +                ChipError::OperationFailed("Gpio Chip find-line", IoError::new(ENOENT))
> > +            );
> > +        }
> 
> If it is testing find_line() then why not call it find_line?

I think I picked many names from the TEST_CASE name from cxx bindings. This was
written there as:

TEST_CASE("line lookup by name works", "[chip]")

and I didn't think much about it :)

Sure I can name this find_line().

> > diff --git a/bindings/rust/tests/edge_event.rs b/bindings/rust/tests/edge_event.rs
> > +        #[test]
> > +        fn wait_timeout() {
> > +            let mut config = TestConfig::new(NGPIO).unwrap();
> > +            config.rconfig(Some(&[0]));
> > +            config.lconfig_edge(Some(Edge::Both));
> > +            config.request_lines().unwrap();
> > +
> > +            // No events available
> > +            assert_eq!(
> > +                config
> > +                    .request()
> > +                    .wait_edge_event(Duration::from_millis(100))
> > +                    .unwrap_err(),
> > +                ChipError::OperationTimedOut
> > +            );
> > +        }
> 
> Is a timeout really a "failure"?
> 
> It is testing wait_edge_event(), which is a method of line_request,
> and so should be in the line_request test suite.

Copied it from cxx again, I just tried to add similar tests in similar files.

TEST_CASE("edge_event wait timeout", "[edge-event]")

> > +
> > +        #[test]
> > +        fn dir_out_edge_failure() {
> > +            let mut config = TestConfig::new(NGPIO).unwrap();
> > +            config.rconfig(Some(&[0]));
> > +            config.lconfig(Some(Direction::Output), None, None, Some(Edge::Both), None);
> > +
> > +            assert_eq!(
> > +                config.request_lines().unwrap_err(),
> > +                ChipError::OperationFailed("Gpio LineRequest request-lines", IoError::new(EINVAL))
> > +            );
> > +        }
> > +    }
> > +
> 
> This is testing a failure mode of request_lines(), not edge_events.
> Where is the edge_event here?

I agree, I will move this out.

This needs fixing too though.

TEST_CASE("output mode and edge detection don't work together", "[edge-event]")

> > diff --git a/bindings/rust/tests/info_event.rs b/bindings/rust/tests/info_event.rs
> > new file mode 100644
> > index 000000000000..96d8385deadf
> > --- /dev/null
> > +++ b/bindings/rust/tests/info_event.rs
> > @@ -0,0 +1,126 @@
> > +// SPDX-License-Identifier: Apache-2.0 AND BSD-3-Clause
> > +//
> > +// Copyright 2022 Linaro Ltd. All Rights Reserved.
> > +//     Viresh Kumar <viresh.kumar@linaro.org>
> > +
> > +mod common;
> > +
> > +mod info_event {
> > +    use libc::EINVAL;
> > +    use std::sync::Arc;
> > +    use std::thread::{sleep, spawn};
> > +    use std::time::Duration;
> > +
> > +    use vmm_sys_util::errno::Error as IoError;
> > +
> > +    use crate::common::*;
> > +    use libgpiod::{Chip, Direction, Error as ChipError, Event, LineConfig, RequestConfig};
> > +
> > +    fn request_reconfigure_line(chip: Arc<Chip>) {
> > +        spawn(move || {
> > +            sleep(Duration::from_millis(10));
> > +
> > +            let lconfig1 = LineConfig::new().unwrap();
> > +            let rconfig = RequestConfig::new().unwrap();
> > +            rconfig.set_offsets(&[7]);
> > +
> > +            let request = chip.request_lines(&rconfig, &lconfig1).unwrap();
> > +
> > +            sleep(Duration::from_millis(10));
> > +
> > +            let mut lconfig2 = LineConfig::new().unwrap();
> > +            lconfig2.set_direction_default(Direction::Output);
> > +
> > +            request.reconfigure_lines(&lconfig2).unwrap();
> > +
> > +            sleep(Duration::from_millis(10));
> > +        });
> > +    }
> > +
> 
> Benefit of the background thread?

Again copied from cxx, I think the idea here is to get the events one by one and
read one before the next one is generated. i.e. to do it all in parallel, which
looked fine to me.

> > +    mod properties {
> > +        use super::*;
> > +
> > +        #[test]
> > +        fn verify() {
> > +            let sim = Sim::new(Some(NGPIO), None, false).unwrap();
> > +            sim.set_line_name(1, "one").unwrap();
> > +            sim.set_line_name(2, "two").unwrap();
> > +            sim.set_line_name(4, "four").unwrap();
> > +            sim.set_line_name(5, "five").unwrap();
> > +            sim.hog_line(3, "hog3", GPIOSIM_HOG_DIR_OUTPUT_HIGH as i32)
> > +                .unwrap();
> > +            sim.hog_line(4, "hog4", GPIOSIM_HOG_DIR_OUTPUT_LOW as i32)
> > +                .unwrap();
> > +            sim.enable().unwrap();
> > +
> > +            let chip = Chip::open(sim.dev_path()).unwrap();
> > +            chip.line_info(6).unwrap();
> > +
> > +            let info4 = chip.line_info(4).unwrap();
> > +            assert_eq!(info4.get_offset(), 4);
> > +            assert_eq!(info4.get_name().unwrap(), "four");
> > +            assert_eq!(info4.is_used(), true);
> > +            assert_eq!(info4.get_consumer().unwrap(), "hog4");
> > +            assert_eq!(info4.get_direction().unwrap(), Direction::Output);
> > +            assert_eq!(info4.is_active_low(), false);
> > +            assert_eq!(info4.get_bias().unwrap(), Bias::Unknown);
> > +            assert_eq!(info4.get_drive().unwrap(), Drive::PushPull);
> > +            assert_eq!(info4.get_edge_detection().unwrap(), Edge::None);
> > +            assert_eq!(info4.get_event_clock().unwrap(), EventClock::Monotonic);
> > +            assert_eq!(info4.is_debounced(), false);
> > +            assert_eq!(info4.get_debounce_period(), Duration::from_millis(0));
> > +        }
> > +    }
> 
> Test that you can read all supported values for all fields.

Like setting bias to all possible values one by one and reading them back ? Or
something else ?

> Clippy warnings to fix:
> 
> $cargo clippy --tests

I just ran

cargo clippy --workspace --bins --examples --benches --all-features -- -D warnings

and thought it works for tests too, my bad.

-- 
viresh