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

[Harold Seigel]

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
>>>>>