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

Wed Feb 20 13:12:48 UTC 2019

Thanks David!

Harold

On 2/19/2019 9:39 PM, David Holmes wrote:
> 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
>>>>>>>