RFR (S) 8167548: [TESTBUG] Logging tests put log files in source tree

[David Holmes]

Hi Harold,

On 16/02/2019 4:39 am, Harold Seigel wrote:
> Hi,
> 
> Please review this updated fix for this logging test issue.  The fix 
> changes tests to create logging files in the temp directory and includes 
> the pid as part of the logging file names.
> 
> Open Webrev: 
> http://cr.openjdk.java.net/~hseigel/bug_8167548.2/webrev/index.html
> 
> JBS Bug: https://bugs.openjdk.java.net/browse/JDK-8167548

It seems extremely complicated for what it is doing ...

> The fix includes the following changes:
> 
>  1. Method cur_log_file_name() was added to the JVM logging support to
>     return the name of current logging file.

I hate seeing this in product code just to support test code, but 
realize the key problem being solved here is ensuring the test knows the 
correct name for the log file.

>  2. Support was added to logTestFixture.cpp to prefix the temp dir and
>     pid to the name of logging files that include test specific information.

Seems very complicated. We create a temporary stack array to write most 
things into; then copy that into a temporary ResourceArray with the tmp 
prefix added, and then copy that into the real destination! Can't we 
simply expand the jio_snprintf with %s%s and pass 
os::get_temp_directory() and os::file_separator() ?

>  3. Method prepend_temp_dir() was added for use by logging tests to
>     prepend temp dir and pid to logging files with simple names, such as
>     "start-rotate-test".
> 
>  4. Method prepend_prefix_temp_dir() was added for use by logging tests
>     to create a malloc allocated string that includes a prefix, such as
>     'file=', the temp dir, and a user specified string.

You define:

   91 static const char* tmp_dir = os::get_temp_directory();
   92 static const char* file_sep = os::file_separator();

but then don't use them where you need them:

  99   int ret = jio_snprintf(temp_file, temp_file_len, "%s%spid%d.%s",
  100                          os::get_temp_directory(), 
os::file_separator(),

110   int ret = jio_snprintf(temp_file, temp_file_len, "%s%s%s%s",
  111                          prefix, os::get_temp_directory(), 
os::file_separator(), filename);


I can't say I particularly like the changes but as it's test code I 
really don't care that much. There are bigger fish to fry.

Thanks,
David
-----

>  5. Test test_logConfiguration needed to be changed to properly handle a
>     logging file that actually begins with "\"file=..."\".
> 
>  6. Other logging tests were changed to call the new methods.
> 
> The changes were tested locally to check that the files were properly 
> named and deleted.  Additionally, the tests were run on on Windows, 
> Linux, and Mac.  Also, the fix was regression tested by running Mach5 
> tiers 1 and 2 tests and builds on Linux-x64, Windows, and Mac OS X, 
> Mach5 tiers 3 -5 on Linux-x64, and by running JCK Lang and VM tests on 
> Linux-x64.
> 
> Thanks, Harold
> 
> 
> On 2/8/2019 10:02 AM, Harold Seigel wrote:
>> Hi David,
>>
>> Thanks for looking at this.
>>
>> The test's logging file name includes the job's start time.  But, the 
>> test's call to LogFileOutput::set_file_name_parameters(0) sets the 
>> job's start time to the beginning of time: 1970-01-01_01-00-00.  The 
>> logging code then converts this from UTC to local time and uses the 
>> local time in the file name.  And, since Burlington MA is five hours 
>> behind UTC, the resulting time used in the file name is: 
>> 1969-12-31_19-00-00.  So, my 'fix' would only work when the test was 
>> run in a timezone that's five hours behind UTC.
>>
>> It looks like many of the gtest logging tests create files in the 
>> source directory.  So, I'm withdrawing this change until this bigger 
>> problem can be addressed.
>>
>> Thanks, Harold
>>
>> On 2/7/2019 7:26 PM, David Holmes wrote:
>>> Hi Harold,
>>>
>>> On 8/02/2019 10:08 am, Harold Seigel wrote:
>>>> Hi,
>>>>
>>>> Please review this fix for JDK-8167548.  The test used the wrong 
>>>> 'predictable' file names so the files were not getting deleted.
>>>
>>> I'm confused. Where does the magic "1969-12-31_19-00-00" date string 
>>> come from?
>>>
>>> Second, this still means the test puts logs into the source tree, 
>>> which is a bad thing. can we not fix this so that the log file is in 
>>> a tmp directory, or a jtreg working directory or ...?
>>>
>>> Thanks,
>>> David
>>>
>>>> Open Webrev: 
>>>> http://cr.openjdk.java.net/~hseigel/bug_8167548/webrev/index.html
>>>>
>>>> JBS Bug: https://bugs.openjdk.java.net/browse/JDK-8167548
>>>>
>>>> The fix was tested by hand on Linux x64 to make sure the files were 
>>>> deleted.  It was also regression tested on Windows, Solaris, and Mac.
>>>>
>>>> Thanks, Harold
>>>>