<AWT Dev> [9] review request for 8072769: System tray icon title freezes java

[Semyon Sadetsky]

Hi Sergey,

Thank you for your response.
I have added max+-1 cases to the test.

http://cr.openjdk.java.net/~alexsch/semyon-sadetsky/8072769/webrev.05/

cases 0,1, etc are not related to the bug. I understand you worries 
about test coverage but I think it's better to follow the process.
You can file a ticket to increase system tray test coverage and we 
immediately start to work on it upon it is prioritized.

--Semyon


On 3/4/2015 11:17 PM, Sergey Bylokhov wrote:
> On 04.03.2015 16:33, Semyon Sadetsky wrote:
>> Sergey,
>> OK. You can file a request to improve TrayIcon test coverage but it's 
>> an another story. Here we are discussing regression test for a 
>> specific bug.
>> I cannot agree with the approach you've proposed. In your logic we 
>> need start to write regression tests for all methods in JDK classes 
>> those take String parameters by iterating all possible parameters 
>> lengths to discover potential bugs.
> Yes, if we know that there is a high probability to crash the method.
>> Tests need to be written by certain rules. Scanning all possible 
>> values of input parameters is not the best rule in my opinion.
> Validation of one number is useless too. Its even does not check all 
> corner cases , 0,1,max-1,max,max+1. so the range is better.
>>
>> --Semyon
>>
>> On 3/4/2015 4:03 PM, Sergey Bylokhov wrote:
>>> On 04.03.2015 15:08, Semyon Sadetsky wrote:
>>>> I think it's more like bug regression test policy. Since I'm a 
>>>> novice in the project maybe you or somebody can advise me the right 
>>>> process.
>>>> Lets see what we have here:
>>>> The fix is connected to the windows platform native code only. It 
>>>> is a platform specific corner case. I would not say it contains 
>>>> "magic numbers", those numbers a not magic for the native layer. 
>>>> But in the test we call native code from java where in truth string 
>>>> parameters maximum lengths are not specified.
>>>> I think it make sense to run the test on all other platforms test 
>>>> iterating strings lengths up to 1000 or more just to ensure that 
>>>> there no similar issues there. We can do this once or have an 
>>>> option to switch this scenario on.
>>> The goal of the test is to catch all possibly related issues. So 
>>> instead of numbers use ranges, cover all platforms if the test don't 
>>> use platform's specific classes, cover all look and feels, automatic 
>>> test is better than manual, make the test generic. But it has of 
>>> course some restrictions like performance and stability. Moreover it 
>>> seems that right now displayMessage/setToolTip are used in the 
>>> manual tests only.
>>>
>>>> But I'm not 100% sure that such test scenario should be included in 
>>>> the regular regression run because sending messages to the system 
>>>> tray is not very fast and the scan can take noticeable amount of 
>>>> time. In my opinion such issue seeking scenario is reasonable to 
>>>> run time to time or as a part of stress testing profile, but it 
>>>> does not make sense to run it during the regular regression because 
>>>> it tests nothing specific on platforms other than Windows and even 
>>>> in Windows there is only one specific lengths combination. It's not 
>>>> worth to do this potential issue seek it's time expensive but will 
>>>> not bring us a lot of value really.
>>>>
>>>> --Semyon
>>>>
>>>> On 3/4/2015 2:13 PM, Sergey Bylokhov wrote:
>>>>> Hi, Semyon.
>>>>> I suggest to remove stuff related to windows platform from the 
>>>>> test. Also it will be good to test some reasonable range of data 
>>>>> instead of magic constant, wrap setTooltip/displayMessage in a loop.
>>>>> Please add "tray.remove(trayIcon);" at the end of the test, 
>>>>> otherwise it can hang, when will be run w/o jtreg.
>>>>>
>>>>> On 03.03.2015 15:32, Semyon Sadetsky wrote:
>>>>>> accepted.
>>>>>> http://cr.openjdk.java.net/~alexsch/semyon-sadetsky/8072769/webrev.03/ 
>>>>>>
>>>>>>
>>>>>>
>>>>>> On 3/3/2015 1:16 PM, Alexander Scherbatiy wrote:
>>>>>>>
>>>>>>>  Just few comments about the test:
>>>>>>>   - The test sets WindowsLookAndFeel and seems fails on non 
>>>>>>> Windows platforms.
>>>>>>>   - There is the second SystemTray.isSupported() check on line 
>>>>>>> 49. Does it depends on L&F?
>>>>>>>   - The copyright should be updated to 2015.
>>>>>>>
>>>>>>>  Thanks,
>>>>>>>  Alexandr.
>>>>>>>
>>>>>>> On 3/2/2015 4:01 PM, Semyon Sadetsky wrote:
>>>>>>>> Hello,
>>>>>>>>
>>>>>>>> Test was added. Please review.
>>>>>>>> webrev: 
>>>>>>>> http://cr.openjdk.java.net/~alexsch/semyon-sadetsky/8072769/webrev.02/ 
>>>>>>>>
>>>>>>>> bug: https://bugs.openjdk.java.net/browse/JDK-8072769
>>>>>>>>
>>>>>>>>
>>>>>>>> On 2/26/2015 10:46 AM, Semyon Sadetsky wrote:
>>>>>>>>> fix updated: 
>>>>>>>>> http://cr.openjdk.java.net/~alexsch/semyon-sadetsky/8072769/webrev.01/ 
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On 2/24/2015 12:12 PM, Semyon Sadetsky wrote:
>>>>>>>>>> Hello,
>>>>>>>>>> <AWT Dev> [9] review request for 8061636: Fix for JDK-7079254 
>>>>>>>>>> changes behavior of MouseListener, MouseMotionListener
>>>>>>>>>> please review the 
>>>>>>>>>> fix:http://cr.openjdk.java.net/~alexsch/semyon-sadetsky/8072769/webrev/ 
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> for the issue:https://bugs.openjdk.java.net/browse/JDK-8072769
>>>>>>>>>>
>>>>>>>>>> System tray icon title freezes java
>>>>>>>>>>
>>>>>>>>>> This fix contains:
>>>>>>>>>> fix corner case: <buffer size> == <length of the string>
>>>>>>>>>> for two string parameters:
>>>>>>>>>> 1. balloon title string
>>>>>>>>>> 2. balloon text string
>>>>>>>>>>
>>>>>>>>>> -- 
>>>>>>>>>> Thanks,
>>>>>>>>>>
>>>>>>>>>> Semyon.
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>>
>>>>
>>>
>>>
>>> -- 
>>> Best regards, Sergey.
>>
>
>
> -- 
> Best regards, Sergey.

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/awt-dev/attachments/20150311/21088449/attachment.html>