dev@javaserverfaces.java.net

Re: Call for code review for JAVASERVERFACES-3334

From: manfred riem <manfred.riem_at_oracle.com>
Date: Wed, 13 Aug 2014 10:49:05 -0500

Hi Zhijun,

Thanks for comitting.

Manfred

On 8/12/14, 8:31 PM, zhijun Ren wrote:
> Hi Manfred,
>
> Code was committed with r13502 after your comment fixed and the bug's
> status is resolved now.
>
> Adding test/glassfish/facelets
> Adding test/glassfish/facelets/core
> Adding test/glassfish/facelets/core/pom.xml
> Adding test/glassfish/facelets/core/src
> Adding test/glassfish/facelets/core/src/main
> Adding test/glassfish/facelets/core/src/main/java
> Adding test/glassfish/facelets/core/src/main/java/com
> Adding test/glassfish/facelets/core/src/main/java/com/sun
> Adding test/glassfish/facelets/core/src/main/java/com/sun/faces
> Adding
> test/glassfish/facelets/core/src/main/java/com/sun/faces/annotation
> Adding
> test/glassfish/facelets/core/src/main/java/com/sun/faces/annotation/AnnotatedValidatorNoValue.java
> Adding
> test/glassfish/facelets/core/src/main/java/com/sun/faces/test
> Adding
> test/glassfish/facelets/core/src/main/java/com/sun/faces/test/glassfish
> Adding
> test/glassfish/facelets/core/src/main/java/com/sun/faces/test/glassfish/facelets
> Adding
> test/glassfish/facelets/core/src/main/java/com/sun/faces/test/glassfish/facelets/core
> Adding
> test/glassfish/facelets/core/src/main/java/com/sun/faces/test/glassfish/facelets/core/AnnotationTestBean.java
> Adding test/glassfish/facelets/core/src/main/webapp
> Adding test/glassfish/facelets/core/src/main/webapp/WEB-INF
> Adding
> test/glassfish/facelets/core/src/main/webapp/WEB-INF/faces-config.xml
> Adding
> test/glassfish/facelets/core/src/main/webapp/WEB-INF/glassfish-web.xml
> Adding
> test/glassfish/facelets/core/src/main/webapp/WEB-INF/web.xml
> Adding
> test/glassfish/facelets/core/src/main/webapp/annotationtest.xhtml
> Adding test/glassfish/facelets/core/src/test
> Adding test/glassfish/facelets/core/src/test/java
> Adding test/glassfish/facelets/core/src/test/java/com
> Adding test/glassfish/facelets/core/src/test/java/com/sun
> Adding test/glassfish/facelets/core/src/test/java/com/sun/faces
> Adding
> test/glassfish/facelets/core/src/test/java/com/sun/faces/test
> Adding
> test/glassfish/facelets/core/src/test/java/com/sun/faces/test/glassfish
> Adding
> test/glassfish/facelets/core/src/test/java/com/sun/faces/test/glassfish/facelets
> Adding
> test/glassfish/facelets/core/src/test/java/com/sun/faces/test/glassfish/facelets/core
> Adding
> test/glassfish/facelets/core/src/test/java/com/sun/faces/test/glassfish/facelets/core/AnnotatedComponentIT.java
> Adding test/glassfish/facelets/pom.xml
> Sending test/glassfish/pom.xml
> Adding test/weblogic/wls1214/facelets
> Adding test/weblogic/wls1214/facelets/core
> Adding test/weblogic/wls1214/facelets/core/pom.xml
> Adding test/weblogic/wls1214/facelets/core/src
> Adding test/weblogic/wls1214/facelets/core/src/main
> Adding test/weblogic/wls1214/facelets/core/src/main/java
> Adding test/weblogic/wls1214/facelets/core/src/main/java/com
> Adding test/weblogic/wls1214/facelets/core/src/main/java/com/sun
> Adding
> test/weblogic/wls1214/facelets/core/src/main/java/com/sun/faces
> Adding
> test/weblogic/wls1214/facelets/core/src/main/java/com/sun/faces/annotation
> Adding
> test/weblogic/wls1214/facelets/core/src/main/java/com/sun/faces/annotation/AnnotatedValidatorNoValue.java
> Adding
> test/weblogic/wls1214/facelets/core/src/main/java/com/sun/faces/test
> Adding
> test/weblogic/wls1214/facelets/core/src/main/java/com/sun/faces/test/weblogic
> Adding
> test/weblogic/wls1214/facelets/core/src/main/java/com/sun/faces/test/weblogic/wls1214
> Adding
> test/weblogic/wls1214/facelets/core/src/main/java/com/sun/faces/test/weblogic/wls1214/facelets
> Adding
> test/weblogic/wls1214/facelets/core/src/main/java/com/sun/faces/test/weblogic/wls1214/facelets/core
> Adding
> test/weblogic/wls1214/facelets/core/src/main/java/com/sun/faces/test/weblogic/wls1214/facelets/core/AnnotationTestBean.java
> Adding test/weblogic/wls1214/facelets/core/src/main/webapp
> Adding
> test/weblogic/wls1214/facelets/core/src/main/webapp/WEB-INF
> Adding
> test/weblogic/wls1214/facelets/core/src/main/webapp/WEB-INF/faces-config.xml
> Adding
> test/weblogic/wls1214/facelets/core/src/main/webapp/WEB-INF/web.xml
> Adding
> test/weblogic/wls1214/facelets/core/src/main/webapp/WEB-INF/weblogic.xml
> Adding
> test/weblogic/wls1214/facelets/core/src/main/webapp/annotationtest.xhtml
> Adding test/weblogic/wls1214/facelets/core/src/test
> Adding test/weblogic/wls1214/facelets/core/src/test/java
> Adding test/weblogic/wls1214/facelets/core/src/test/java/com
> Adding test/weblogic/wls1214/facelets/core/src/test/java/com/sun
> Adding
> test/weblogic/wls1214/facelets/core/src/test/java/com/sun/faces
> Adding
> test/weblogic/wls1214/facelets/core/src/test/java/com/sun/faces/test
> Adding
> test/weblogic/wls1214/facelets/core/src/test/java/com/sun/faces/test/weblogic
> Adding
> test/weblogic/wls1214/facelets/core/src/test/java/com/sun/faces/test/weblogic/wls1214
> Adding
> test/weblogic/wls1214/facelets/core/src/test/java/com/sun/faces/test/weblogic/wls1214/facelets
> Adding
> test/weblogic/wls1214/facelets/core/src/test/java/com/sun/faces/test/weblogic/wls1214/facelets/core
> Adding
> test/weblogic/wls1214/facelets/core/src/test/java/com/sun/faces/test/weblogic/wls1214/facelets/core/AnnotatedComponentIT.java
> Adding test/weblogic/wls1214/facelets/pom.xml
> Sending test/weblogic/wls1214/pom.xml
> Transmitting file data ....................
> Committed revision 13502.
>
> BR,
> Zhijun
>
> On 8/12/14, 22:30, manfred riem wrote:
>> Hi Zhijun,
>>
>> Change jsf-glassfish-facelets-core to test-glassfish-facelets-core
>>
>> Then go ahead and commit it
>>
>> Thanks!
>> Manfred
>>
>> On 8/10/14, 10:07 PM, zhijun Ren wrote:
>>> Hi Manfred,
>>>
>>> There are many unrelated property changed entries in the change
>>> bundle, I reverted them and updated the changed bundle.
>>>
>>> Thanks,
>>> Zhijun
>>>
>>> On 8/11/14, 9:49, zhijun Ren wrote:
>>>> Hi Manfred,
>>>>
>>>> Valuable comments for me, those are rules for me to abide by in the
>>>> future, thanks.
>>>>
>>>> Fixed and attach the new change bundle for further review.
>>>>
>>>> BR,
>>>> Zhijun
>>>>
>>>>
>>>> On 8/8/14, 21:30, manfred riem wrote:
>>>>> Hi Zhijun,
>>>>>
>>>>> Sorry to ask for another change but can you change the following?
>>>>>
>>>>> The <finalName>section and the glassfish-web.xml need to match the
>>>>> <name> section.
>>>>>
>>>>> Eg. <finalName>jsf-glassfish-AnnotationValidatorTest</finalName>
>>>>> should be
>>>>> <finalName>test-glassfish-facelets-core</finalName>
>>>>>
>>>>> And the glassfish-web.xml should contain
>>>>> /test-glassfish-facelets-core
>>>>>
>>>>> And similarly for the second project, but then for weblogic (with
>>>>> a weblogic.xml)
>>>>>
>>>>> Thanks!
>>>>> Manfred
>>>>>
>>>>> On 8/8/14, 3:49 AM, zhijun Ren wrote:
>>>>>> Hi Manfred,
>>>>>>
>>>>>> Complete to change to use the new style testing. new change
>>>>>> bundle attached here.
>>>>>>
>>>>>> I have a question about module test/weblogic, the projects under
>>>>>> it were never executed by any Hudson jobs, right?
>>>>>>
>>>>>> BR,
>>>>>> Zhijun
>>>>>>
>>>>>>
>>>>>>
>>>>>> On 8/8/14, 3:16, manfred riem wrote:
>>>>>>> Hi Zhijun,
>>>>>>>
>>>>>>> Almost there, please remove the usage of HtmlUnitFacesITCase and
>>>>>>> use the new style testing that
>>>>>>> uses annotations.
>>>>>>>
>>>>>>> Thanks!
>>>>>>> Manfred
>>>>>>>
>>>>>>> On 8/7/14, 12:55 AM, zhijun Ren wrote:
>>>>>>>> Hi Manfred,
>>>>>>>>
>>>>>>>> Thanks for your comments. I have fixed them, attach the updated
>>>>>>>> changebundle and newfiles here for your double check.
>>>>>>>>
>>>>>>>> BR,
>>>>>>>> Zhijun
>>>>>>>>
>>>>>>>> On 8/7/14, 1:55, manfred riem wrote:
>>>>>>>>> Hi Zhijun,
>>>>>>>>>
>>>>>>>>> Can you implement the following changes and send out another
>>>>>>>>> changebundle?
>>>>>>>>>
>>>>>>>>> 1. Please order the modules section in the POMs alphabetically.
>>>>>>>>> 2. Please do not include new dependencies in the dependency
>>>>>>>>> section.
>>>>>>>>> 3. Please make sure the web.xml files contain the
>>>>>>>>> ${webapp.xxxx} parameters,
>>>>>>>>>
>>>>>>>>> <context-param>
>>>>>>>>> <param-name>javax.faces.PROJECT_STAGE</param-name>
>>>>>>>>> <param-value>${webapp.projectStage}</param-value>
>>>>>>>>> </context-param>
>>>>>>>>> <context-param>
>>>>>>>>> <param-name>javax.faces.PARTIAL_STATE_SAVING</param-name>
>>>>>>>>> <param-value>${webapp.partialStateSaving}</param-value>
>>>>>>>>> </context-param>
>>>>>>>>> <context-param>
>>>>>>>>> <param-name>javax.faces.STATE_SAVING_METHOD</param-name>
>>>>>>>>> <param-value>${webapp.stateSavingMethod}</param-value>
>>>>>>>>> </context-param>
>>>>>>>>>
>>>>>>>>> 4. Change AnnotatedComponentsITCase to AnnotatedComponentIT
>>>>>>>>> and use
>>>>>>>>> the new testing pattern, see any of the projects for an
>>>>>>>>> example on that, or
>>>>>>>>> if you need help let me know.
>>>>>>>>>
>>>>>>>>> 5. Fix the package names to reflect which project they are in
>>>>>>>>> (this makes it
>>>>>>>>> easier to figure out which project a given class belongs
>>>>>>>>> to when you look
>>>>>>>>> at the test report).
>>>>>>>>>
>>>>>>>>> Eg.
>>>>>>>>> com.sun.faces.test.weblogic.wls1214.facelets.core.AnnotationTestBean
>>>>>>>>>
>>>>>>>>> for the code in test/weblogic/wls1214/facelets/core
>>>>>>>>>
>>>>>>>>> Do that both for the tests and the managed beans.
>>>>>>>>>
>>>>>>>>> 6. When using HtmlUnit please do not use the asText() method,
>>>>>>>>> but always use asXml().
>>>>>>>>>
>>>>>>>>> Thanks!
>>>>>>>>> Manfred
>>>>>>>>>
>>>>>>>>> On 8/6/14, 3:16 AM, zhijun Ren wrote:
>>>>>>>>>> Hi Manfred and Ed,
>>>>>>>>>>
>>>>>>>>>> Please help to do code review for my change for JIRA3334,
>>>>>>>>>> attached the changebundle.txt and the newfiles.zip.
>>>>>>>>>>
>>>>>>>>>> Main changes:
>>>>>>>>>> 1. Only copy the AnnotatedValidatorNoValue Test logic by
>>>>>>>>>> rewrite the web project and related test classes;
>>>>>>>>>> 2. Uncomment the @FacesValidator in AnnotatedValidatorNoValue;
>>>>>>>>>> 3. The tests are in test/glassfish and
>>>>>>>>>> test/weblogic/wls11214 now;
>>>>>>>>>>
>>>>>>>>>> Thanks for any comments.
>>>>>>>>>>
>>>>>>>>>> Zhijun