Hi Ed,
Please help to review the following changes(changebundle attached), once
you approved I will do delivery to make the Hudson build clean:
1. Putting back your additional changes in NavigationHandlerImpl but
with a little change of me, because
com.sun.faces.test.javaee6web.flowinxmlandjava.Spec730IT failed at a
StackOverFlowExcetion, the little change is in method findSwitchMatch()
as following:
/*boolean matched=false;*
for (SwitchCase cur : cases) {
if (cur.getCondition(context)) {
outcome = cur.getFromOutcome();
*matched=true;*
break;
}
}
// 2. the default outcome
if (*!matched || *outcome == null) {
outcome = switchNode.getDefaultOutcome(context);
}/
2. My another change is in
test/javaee6web/flowSwitchCall/src/main/java/com/sun/faces/test/javaee6web/flowswitchcall/FlowA.java
because of the typos of the original test author.
3. My third change is that the following 2 test cases(of that 25
navigation cases) are commented out because of the failure, I will
uncomment them once the issue are fixed.
com.sun.faces.test.javaee6web.flowtraversalcombinations.ReturnNaviToOtherNodesIT#testReturnNaviToFlowCallNode
and
com.sun.faces.test.javaee6web.flowtraversalcombinations.SwitchNaviToOtherNodesIT#testSwitchNaviToReturn
There will be no failed tests related to my change after above change
committed cause 2 failed tests are removed.
BR,
Zhijun
On 1/21/15, 2:47, Edward Burns wrote:
> Hello Zhijun,
>
> Comments below.
>
>>>>>> On Mon, 19 Jan 2015 16:50:51 +0800, zhijun Ren<ren.zhijun_at_oracle.com> said:
> ZR> Hi Ed and Manfred,
> ZR> I have committed the code, I need note something here to avoid surprising:
>
> Thanks. Let me say first that you have done exactly as we agreed in the
> meeting on Thursday.
>
> ZR> 1. For the fixing code in class NavigationHandlerImpl, I only keep the
> ZR> code that handle case that start from method call node and removed the
> ZR> code that handles the case start from switch node due to it fails the
> ZR> cases which starting from switch node both legacy and newly-added;
>
> Due to the noted complexity of this code, I'm going to ask that you put
> the removed code back, and hopefully it won't break any additional
> tests. If it does we must roll back the entire commit r14211. Having
> the code there may aid in the refactoring process which I discuss later
> in this email.
>
> ZR> 2. I have do the Hudson testing against weblogic and the failed tests are:
> ZR> /./javaee6web/flowTraversalCombinations/target/failsafe-reports/com.sun.faces.test.javaee6web.flowtraversalcombinations.ReturnNaviToOtherNodesIT.txt:Tests
> ZR> run: 5, Failures: 0, Errors: 1, Skipped: 0, Time elapsed: 0.263 sec<<<
> ZR> FAILURE! - in
> ZR> com.sun.faces.test.javaee6web.flowtraversalcombinations.ReturnNaviToOtherNodesIT
> ZR> ./javaee6web/flowTraversalCombinations/target/failsafe-reports/com.sun.faces.test.javaee6web.flowtraversalcombinations.SwitchNaviToOtherNodesIT.txt:Tests
> ZR> run: 5, Failures: 4, Errors: 0, Skipped: 0, Time elapsed: 0.167 sec<<<
> ZR> FAILURE! - in
> ZR> com.sun.faces.test.javaee6web.flowtraversalcombinations.SwitchNaviToOtherNodesIT
> ZR> ./javaee7/cdiBeanValidator/target/failsafe-reports/com.sun.faces.test.javaee7.cdibeanvalidator.Issue3014IT.txt:Tests
> ZR> run: 1, Failures: 1, Errors: 0, Skipped: 0, Time elapsed: 1.179 sec<<<
> ZR> FAILURE! - in com.sun.faces.test.javaee7.cdibeanvalidator.Issue3014IT
> ZR> /
>
> ZR> 3. I have do the Hudson testing against glassfish and the failed tests are:
> ZR> /./javaee6web/flowTraversalCombinations/target/failsafe-reports/com.sun.faces.test.javaee6web.flowtraversalcombinations.ReturnNaviToOtherNodesIT.txt:Tests
> ZR> run: 5, Failures: 0, Errors: 1, Skipped: 0, Time elapsed: 0.231 sec<<<
> ZR> FAILURE! - in
> ZR> com.sun.faces.test.javaee6web.flowtraversalcombinations.ReturnNaviToOtherNodesIT
> ZR> ./javaee6web/flowTraversalCombinations/target/failsafe-reports/com.sun.faces.test.javaee6web.flowtraversalcombinations.SwitchNaviToOtherNodesIT.txt:Tests
> ZR> run: 5, Failures: 4, Errors: 0, Skipped: 0, Time elapsed: 0.166 sec<<<
> ZR> FAILURE! - in
> ZR> com.sun.faces.test.javaee6web.flowtraversalcombinations.SwitchNaviToOtherNodesIT/
>
> ACTION: => For your items 2 and 3, I know I had directed you to commit
> these known failing tests. Instead of doing that, please svn remove
> them and re-add them as you make them work while fixing issues 3694 and
> 3695.
>
> ZR> 4. Cause I didn't find failed test cases starting from method call node,
> ZR> so I set the bug #3593 as resolved;
>
> Yes, we agreed to this.
>
> ZR> 5. I filed jira bugs for the failed Tests:
> ZR> JAVASERVERFACES-3694
> ZR> <https://java.net/jira/browse/JAVASERVERFACES-3694>: for
> ZR> com.sun.faces.test.javaee6web.flowtraversalcombinations.ReturnNaviToOtherNodesIT.testReturnNaviToFlowCallNode
> ZR> JAVASERVERFACES-3695
> ZR> <https://java.net/jira/browse/JAVASERVERFACES-3695>: for tests in
> ZR> com.sun.faces.test.javaee6web.flowtraversalcombinations.SwitchNaviToOtherNodesIT.txt/
>
> We agreed to this as well.
>
> ZR> /6. about com.sun.faces.test.javaee7.cdibeanvalidator.Issue3014IT.txt
> ZR> for Weblogic, I will observe the result in Hudson today, if still
> ZR> failed, I will file a bug also.
>
> Can you please explain how you came upon this observation? Was it
> failing before and now suddenly started working?
>
>>>>>> On Tue, 20 Jan 2015 14:14:18 +0800, zhijun Ren<ren.zhijun_at_oracle.com> said:
> ZR> When I tried to fix #3694 and #3695, I found there are lot bad smells in
> ZR> NavigationHandlerImpl.java, such as duplicated code and many if-else
> ZR> statements.
>
> Yes, you are right. The flow related code in NavigationHandler is
> indeed a source of technical debt. I agree it needs to be refactored
> into a state machine.
>
> ZR> The biggest concern for me is there is many recursive calls, directly or
> ZR> indirectly, which can easily cause Stack Overflow error or exceptions.
> ZR> The code does not obey OO design principles and is very fragile.
>
> Yes, I agree, it is fragile.
>
> ZR> So I think we need to refactor the code with State Design Pattern and
> ZR> other Patterns to remove the bad smells, do you agree?
>
> Yes. I would like to do this work in two big stages, with sub-stages as
> well.
>
> I. Incremental improvement: do this now
>
> 0. Put back removed code from commit of 3593, assuming it doesn't break
> any additional tests.
>
> 1. Get 3694 and 3695 working
>
> 2. Make it so there are no @Ignored flow related tests on trunk.
>
> II. Flow refactoring: do this later, in concert with MVC
>
> Zhijun, we are trying to see if the Faces Flow feature can be re-used
> via CDI from MVC. This work is still in the early discussion phase.
> It makes the most sense to do this technical-debt reducing refactoring
> in concert with this effort. However, at this point, I can still see
> some early work that can be done now.
>
> 1. Come up with a brief list of requirements. Chief among them must
> be the need to not change the behavior of the NavigationHandler for
> non flow related invocations. An equally important requirement is
> the need for the solution to work outside of JSF in MVC.
>
> 2. Come up with a state machine design that does work within the "JSF
> only" context, as a fallback in case the flow feature is dropped
> from MVC.
>
> So, please do take care of part I above now.
>
> Thanks, Zhijun.
>
> Ed
>