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
--
| edward.burns_at_oracle.com | office: +1 407 458 0017
| 33 days til DevNexus 2015
| 43 days til JavaLand 2015
| 53 days til CONFESS 2015