[rfc][icedtea-web] Refactor of LiveConnect Tests Version 2

[Andrew Azores]

On 06/05/2014 10:41 AM, Andrew Azores wrote:
> On 06/05/2014 10:32 AM, Jie Kang wrote:
>> Hello,
>>
>>>> ----- Original Message -----
>>>>> Hello,
>>>>>
>>>>> This patch refactors the majority of LiveConnect Tests (JS to Java 
>>>>> and
>>>>> vice-versa) fixing the race condition between asynchronous JS and the
>>>>> J-applet. Code format is standardized and deprecated code usage is
>>>>> removed/replaced. Note that the file JSTest.js is duplicated for all
>>>>> LiveConnect Tests. This is because every single test requires the 
>>>>> same
>>>>> function in JSTest.js to prevent the race condition from occurring.
>>>>> However,
>>>>> the test suite is not designed to easily have this code appear 
>>>>> only once
>>>>> while also having tests be properly separate (ie. every single 
>>>>> test-case
>>>>> should be able to run by itself without relying on another test 
>>>>> case). If
>>>>> anyone has any suggestions on how to better implement this please 
>>>>> say.
>>>>>
>>>>>
>>>>> Thanks,
>>>>>
>>>>> Jie Kang
>>> I haven't looked over the whole patch yet, but as far as the Makefile
>>> changes go the first thing I notice is that your new stamp's name is a
>>> bit confusing. Why does it contain the word 'compile'? For example, 
>>> look
>>> at the stamp name 'copy-reproducers-resources'. Perhaps this new stamp
>>> can be called just 'copy-shared-reproducers-resources' instead?
>>>
>> The naming was a mistake on my part. I originally intended to have 
>> shared resources include Java files as well hence the need for 
>> compilation but later on decided this wasn't feasible. I have renamed 
>> it to copy-shared-resources.
>>
>>> +    cp -r "$(REPRODUCERS_SHAREDSRCDIR)/resources/"*
>>> $(REPRODUCERS_TESTS_SERVER_DEPLOYDIR)/shared/  ; \
>>>
>>>
>>> Is there a particular reason that you have the asterisk outside of the
>>> quotes in that first argument? Not wrong, just a little strange.
>>> Secondly, you should probably quote the entire second argument. What
>>> happens to your stamp right now if REPRODUCERS_TESTS_SERVER_DEPLOYDIR
>>> gets redefined such that its name contains a space?
>> Using cp "*" versus cp * has different end-result so putting * in the 
>> "" ends up not working. See:
>> cp -R "$(REPRODUCERS_TESTS_SRCDIR)/$$which/$$dir/resources/"* 
>> $(REPRODUCERS_TESTS_SERVER_DEPLOYDIR)/ ; \
>> from 'stamps/copy-reproducers-resources.stamp'.
>
> D'oh, of course. My mistake, sorry.
>
>>
>> I added the quotes to the last argument as suggested.
>>
>>> I don't know if Omair had anything in particular in mind for this
>>> "shared resources" directory but this looks like it's on the right 
>>> track
>>> to me.
>>>
>>> Thanks,
>>>
>>> -- 
>>> Andrew A
>>>
>>>
>>
>> Thanks
>>
>> Jie
>
> This looks better, but it'll take some time for me to look through it 
> all thoroughly.
>
> Also, I should've caught this in the last mail I sent, but please be 
> sure your ChangeLog entries adhere to the format. There should be a 
> space between each * and the file names following it.
>
> Thanks,
>

I ran the reproducer suite with this patch applied last night. There are 
still some tests which are failing but not marked as KnownToFail. I 
haven't checked if these failures are repeatable however. Can you look 
into this?

JavascriptSetTest - AppletJToJSSet_2DArrayElement_Test
JSToJFuncResolTest - AppletJSToJFuncResol_inheritedClassToParent1_Test
JavascriptFuncReturnTest - 
AppletJToJSFuncReturn_{number,boolean,JSObject,String,Object}_Test

Thanks,

-- 
Andrew A