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

[David Holmes]

Looks good Harold! Thanks for making those changes.

David

On 20/02/2019 6:32 am, Harold Seigel wrote:
> Hi David,
> 
> Thanks for looking at this!
> 
> Please review this new update:
> 
>     http://cr.openjdk.java.net/~hseigel/bug_8167548.3/webrev/
> 
> It contains changes to logTestFixture.cpp to use jio_snprintf(), as you 
> suggested.
> 
> Test logTestUtils.inline.hpp was also changed (based on your suggestion) 
> to use the statics tmp_dir and file_sep.
> 
> Thanks, Harold
> 
> On 2/19/2019 12:43 AM, David Holmes wrote:
>> 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
>>>>>>