Re: [libgpiod v2][PATCH v2 2/2] tests: rewrite core C tests using libgpiosim

[Kent Gibson <warthog618@gmail.com>]

On Thu, Feb 24, 2022 at 09:33:40PM +0100, Bartosz Golaszewski wrote:
> On Wed, Feb 23, 2022 at 11:19 AM Kent Gibson <warthog618@gmail.com> wrote:
> >
> > On Mon, Feb 21, 2022 at 04:40:55PM +0100, Bartosz Golaszewski wrote:
> > > This replaces the old tests for the C API v1 based on gpio-mockup with
> > > a test suite based on gpio-sim that covers around 95% of the libgpiod v2
> > > codebase.
> > >
> > > The test harness has been rebuilt and shrank significantly as well. The
> > > libgpiosim API has been wrapped in a gobject interface.
> > >
> >
> > I was having trouble running the tests in a qemu instance where entropy
> > is slow to collect.  The error I get is
> >
> > Bail out! gpiod-test-FATAL-ERROR: Unable to instantiate new GPIO device: Resource temporarily unavailable
> >
> > which I backtracked to the getrandom() call in make_random_dir_at().
> >
> > I have a dislike for random elements in tests as it negatively impacts
> > repeatability.  They are only used here for chip and bank names in the
> > configfs, and otherwise have no bearing on the tests?
> > Why not deterministic naming, say using the test case name, pid,...
> >
> 
> We can have multiple random names in use at once. And on the off
> chance that something goes wrong, a random configfs dir just stays
> there while a deterministic one can hose other tests. Can't you just
> install havaged in whatever rootfs you're using?
> 

Sure, but incorporating the PID in the names would serve a similar
purpose, and also provide some indication as to what created the sim in
the first place.  Random is just noise.  How can you tell if your
configfs objects are orphaned?
Are you sure you aren't biased towards random as it also simplifies your
test setup logic (no pesky names to worry about - just pass a NULL)?

My test setup is a pretty vanilla buildroot/qemu setup configured to
build local kernels and libgpiod, repeated for a few different platforms.
Though in this instance I was only building for x86_64.
No network interfaces, as I'm only testing GPIO, but that also means few
sources of entropy.  So far that hadn't been an issue.
Hadn't heard of haveged, but will give it a go.
<a few minutes later>
That works.  Cool.  Though I still don't like the randomness.

> [snip]
> 
> > >
> > >  #define MIN_KERNEL_MAJOR     5
> > > -#define MIN_KERNEL_MINOR     10
> > > +#define MIN_KERNEL_MINOR     16
> >
> > Might as well bump this to 17 as RCs are available and 16 isn't really
> > sufficient.  Similarly gpiosim.c.
> >
> 
> I'm building this on my laptop so prefer to have it at 16 for now.
> We'll have 5.17 out in three weeks, I'll bump it then.
> 

Not sure why that prevents you from testing against 5.17-rc5, or one of
the earlier RCs, but ok.

> [snip]
> 
> > > +
> > > +GPIOD_TEST_CASE(request_reconfigure_release_events)
> > > +{
> > > +     static const guint offset = 3;
> > > +
> > > +     g_autoptr(GPIOSimChip) sim = g_gpiosim_chip_new("num-lines", 8, NULL);
> > > +     g_autoptr(struct_gpiod_chip) chip = NULL;
> > > +     g_autoptr(struct_gpiod_line_info) info = NULL;
> > > +     g_autoptr(struct_gpiod_info_event) request_event = NULL;
> > > +     g_autoptr(struct_gpiod_info_event) reconfigure_event = NULL;
> > > +     g_autoptr(struct_gpiod_info_event) release_event = NULL;
> > > +     g_autoptr(struct_gpiod_request_config) req_cfg = NULL;
> > > +     g_autoptr(struct_gpiod_line_config) line_cfg = NULL;
> > > +     g_autoptr(GThread) thread = NULL;
> > > +     struct gpiod_line_info *request_info, *reconfigure_info, *release_info;
> > > +     guint64 request_ts, reconfigure_ts, release_ts;
> > > +     struct request_ctx ctx;
> > > +     gint ret;
> > > +
> > > +     chip = gpiod_test_open_chip_or_fail(g_gpiosim_chip_get_dev_path(sim));
> > > +     req_cfg = gpiod_test_create_request_config_or_fail();
> > > +     line_cfg = gpiod_test_create_line_config_or_fail();
> > > +
> > > +     gpiod_request_config_set_offsets(req_cfg, 1, &offset);
> > > +
> > > +     info = gpiod_chip_watch_line_info(chip, 3);
> > > +     g_assert_nonnull(info);
> > > +     gpiod_test_return_if_failed();
> > > +
> > > +     g_assert_false(gpiod_line_info_is_used(info));
> > > +
> > > +     ctx.chip = chip;
> > > +     ctx.req_cfg = req_cfg;
> > > +     ctx.line_cfg = line_cfg;
> > > +
> > > +     thread = g_thread_new("request-release", request_release_line, &ctx);
> > > +     g_thread_ref(thread);
> > > +
> > > +     ret = gpiod_chip_info_event_wait(chip, 1000000000);
> > > +     g_assert_cmpint(ret, >, 0);
> > > +     gpiod_test_join_thread_and_return_if_failed(thread);
> > > +
> > > +     request_event = gpiod_chip_info_event_read(chip);
> > > +     g_assert_nonnull(request_event);
> > > +     gpiod_test_join_thread_and_return_if_failed(thread);
> > > +
> > > +     g_assert_cmpint(gpiod_info_event_get_event_type(request_event), ==,
> > > +                     GPIOD_INFO_EVENT_LINE_REQUESTED);
> > > +
> > > +     request_info = gpiod_info_event_get_line_info(request_event);
> > > +
> > > +     g_assert_cmpuint(gpiod_line_info_get_offset(request_info), ==, 3);
> > > +     g_assert_true(gpiod_line_info_is_used(request_info));
> > > +     g_assert_cmpint(gpiod_line_info_get_direction(request_info), ==,
> > > +                     GPIOD_LINE_DIRECTION_INPUT);
> > > +
> > > +     ret = gpiod_chip_info_event_wait(chip, 1000000000);
> > > +     g_assert_cmpint(ret, >, 0);
> > > +     gpiod_test_join_thread_and_return_if_failed(thread);
> > > +
> > > +     reconfigure_event = gpiod_chip_info_event_read(chip);
> > > +     g_assert_nonnull(reconfigure_event);
> > > +     gpiod_test_join_thread_and_return_if_failed(thread);
> > > +
> > > +     g_assert_cmpint(gpiod_info_event_get_event_type(reconfigure_event), ==,
> > > +                     GPIOD_INFO_EVENT_LINE_CONFIG_CHANGED);
> > > +
> > > +     reconfigure_info = gpiod_info_event_get_line_info(reconfigure_event);
> > > +
> > > +     g_assert_cmpuint(gpiod_line_info_get_offset(reconfigure_info), ==, 3);
> > > +     g_assert_true(gpiod_line_info_is_used(reconfigure_info));
> > > +     g_assert_cmpint(gpiod_line_info_get_direction(reconfigure_info), ==,
> > > +                     GPIOD_LINE_DIRECTION_OUTPUT);
> > > +
> > > +     ret = gpiod_chip_info_event_wait(chip, 1000000000);
> > > +     g_assert_cmpint(ret, >, 0);
> > > +     gpiod_test_join_thread_and_return_if_failed(thread);
> > > +
> > > +     release_event = gpiod_chip_info_event_read(chip);
> > > +     g_assert_nonnull(release_event);
> > > +     gpiod_test_join_thread_and_return_if_failed(thread);
> > > +
> > > +     g_assert_cmpint(gpiod_info_event_get_event_type(release_event), ==,
> > > +                     GPIOD_INFO_EVENT_LINE_RELEASED);
> > > +
> > > +     release_info = gpiod_info_event_get_line_info(release_event);
> > > +
> > > +     g_assert_cmpuint(gpiod_line_info_get_offset(release_info), ==, 3);
> > > +     g_assert_false(gpiod_line_info_is_used(release_info));
> > > +
> > > +     g_thread_join(thread);
> > > +
> > > +     request_ts = gpiod_info_event_get_timestamp(request_event);
> > > +     reconfigure_ts = gpiod_info_event_get_timestamp(reconfigure_event);
> > > +     release_ts = gpiod_info_event_get_timestamp(release_event);
> > > +
> > > +     g_assert_cmpuint(request_ts, <, reconfigure_ts);
> > > +     g_assert_cmpuint(reconfigure_ts, <, release_ts);
> > > +}
> >
> > Is multi-threading really necessary here (and elsewhere, but this the
> > first case where the secondary thread is ALSO asserting)?
> > Couldn't you provide the stimulous and check the result from the one
> > thread?
> 
> I think it's a good way to test a real-life situation. I agree that
> they are not technically required but I'd still keep them, they don't
> hurt.
> 

Sure, and that is why I didn't have a problem with the earlier
instances.  But here you have both threads asserting, so splitting the
test logic.  That crosses a line for me.  Dumb helper threads I'm ok
with.

Cheers,
Kent.