Re: [GNUnet-developers] Broken udp plugin? ARM test fails

[Christian Grothoff]

On 04/24/2015 02:57 AM, LRN wrote:
> The commit does not touch udp, but does change test_gnunet_service_arm. It's
> full of style fixes (hey, grothoff! Thank you for mixing style fixes and
> meaningful code changes!), but does have this noticeable chunk of code 
> removed:
> 
> -  char *armconfig;
> -
> -  if (NULL != cfgfile)
> -  {
> -    if (GNUNET_OK !=
> -        GNUNET_CONFIGURATION_get_value_filename (c, "arm", "CONFIG",
> -                                                 &armconfig))
> -    {
> -      GNUNET_CONFIGURATION_set_value_string ((struct
> GNUNET_CONFIGURATION_Handle *) c,
> -                                             "arm", "CONFIG",
> -                                             cfgfile);
> -    }
> -    else
> -      GNUNET_free (armconfig);
> -  }
> 
> No explanation is given other than "simplify test, log failure cause" (hey,
> grothoff! Thank you for almost never leaving descriptive commit messages!).

Well, as far as I see/saw it, the point of removing this logic was that
as far as I could tell it should not do anything (meaningful anymore),
and removing dead logic is a way to simplify the testcase.  So from my
understanding at the time, I do think the log message was apt.

But I looked some more and now understand the real issue: on GNU/Linux,
the test uses the default UNIXPATH, which was the same between arm.conf
and the test configuration file.  OTOH, on W32, the PORT option was
used, which was different between the two.  Thus, on GNU/Linux the
change did (seemingly) nothing, while on W32 it broke the test. So first
thing to do is to avoid this dichotomy: I removed the PORT for the
test.conf.

> Note that other ARM tests do have this chunk of code.

I was looking at this one, because it was failing on Guix, not at the
other tests at the time, so I did not look at those.  Also, for the
other tests, there are other configuration options that come into play.

I think instead of putting the logic back into this testcase -- where it
is very counter-intuitive and modifies a 'const' struct on top of it, we
should probably have it in util with a big fat comment. Also, the
specific test was written against an ancient config-API and can be done
much nicer now anyway.

0xE29FC3CC.asc
Description: application/pgp-keys