Re: [lmi] Running unit tests in less time

[Greg Chicares]

[quoting everything because this is several months old and full
context makes it clearer]

On 2017-01-11 18:30, Vadim Zeitlin wrote:
> On Wed, 11 Jan 2017 10:12:54 +0000 Greg Chicares <address@hidden> wrote:
> 
> GC> And just now I got this:
> GC> 
> GC> Running mc_enum_test:
> GC> 
> GC> ** uncaught exception: std::exception: boost::filesystem::is_directory: 
> "Z:\opt\lmi\src\build\lmi\Linux\gcc\ship\eraseme.ndx": File not found.
> GC> 
> GC> **** returning with error code 200
> GC> **********  errors detected; see stdout for details  ***********
> GC> /opt/lmi/src/lmi/workhorse.make:1141: recipe for target 
> 'mc_enum_test.exe-run' failed
> GC> 
> GC> That seems puzzling: 'mc_enum_test' shouldn't be looking for any
> GC> rate_table database. (Product database, yes, because it tests
> GC> 'ce_product_name'; but rate_table database, no.)
> 
>  I think the remark about ce_product_name explains it: what seems to happen
> is that there is a race condition in fetch_product_names(). It calls
> is_directory() for all directory entries, but while it's running another
> test can remove a temporary file, such as eraseme.ndx, that it created,
> which results in calling is_directory() on a non-existent file which fails.
> 
>  The simplest fix I can suggest is:
> ---------------------------------- >8 --------------------------------------
> diff --git a/ce_product_name.cpp b/ce_product_name.cpp
> index 09f7466..dbddc33 100644
> --- a/ce_product_name.cpp
> +++ b/ce_product_name.cpp
> @@ -45,7 +45,7 @@ std::vector<std::string> fetch_product_names()
>      fs::directory_iterator end_i;
>      for(; i != end_i; ++i)
>          {
> -        if(is_directory(*i) || ".policy" != fs::extension(*i))
> +        if(".policy" != fs::extension(*i) || is_directory(*i))
>              {
>              continue;
>              }
> ---------------------------------- >8 --------------------------------------

Committed 716aec8; to be pushed later.

> as this would skip the potentially throwing check on anything but .policy
> files and so, assuming no other test creates temporary .policy files,
> should fix the problem. I think it should also be a nice micro-optimization
> as checking the file extension (that we already have) should surely be much
> faster than going back to the file system to check what kind of entry is
> this.

Yes, it should be much faster, and that helps lmi in general. It doesn't
seem ideal to attack a race condition by making it less likely to arise;
but it should arise only in unit tests (never in production for end users),
and perhaps C++17's std::experimental::filesystem will be thread-safe...no,
looks like it won't be:
  http://en.cppreference.com/w/cpp/experimental/fs
| The behavior is undefined if the calls to functions in this library
| introduce a file system race

>  The cleanest/ideal fix would probably be to avoid creating temporary files
> in the current directory and do it in (a subdirectory of) the system
> temporary directory. Please let me know if you'd like me to implement this
> instead of/in addition to the fix above.

Let's see...mktemp() has come to be considered unsafe since the last time I
used it, which was probably in the 1980s. Now mkstemp() and mkdtemp() are
recommended instead, but they're not standard C(++). The standard language
offers tmpfile(), but that removes the file when its handle is closed, which
is too soon for our purposes here; so I guess we're left with tmpnam().
However, tmpnam() is open to race conditions, so we'd probably be better off
hardcoding unique names for each unit test's temporary files (instead of
hardcoding non-unique names as at present). I think that's what I'll do.

Would it really be better to use TMPDIR? If we write temporary files in the
current directory, then we can run i686 and x86_64 tests simultaneously (in
different directories), without any possibility of collision between
architectures.