>>>>> On Fri, 27 Aug 2010 10:34:06 -0700, Sheetal Vartak <sheetal.vartak_at_oracle.com> said:
SV> Thanks for your comments. Attaching the changes incorporated with
SV> the feedback. Please review.
Summary: It's getting there, but not production ready yet. please read
these comments and produce another iteration of the changebundle.
SECTION: Review on the testcase portion of the change bundle
I hate to do this, because I know you want to get it in, but I really
need an automated test to be included as well.
The good news is that it's not hard to add one in this case. I'll
sketch it out for you here. I hope this proves useful to you in
general. If so, please consider adding some content to the Mojarra FAQ.
1. Look for an existing test that does something similar
I thought there might be an existing test that deals with other
ProjectStage==Development features. I know that one of those features
deals with messsages, so I ran a find command in jsf-ri/systest
looking for .java files containing the string "messages". I found
com/sun/faces/systest/NavigationTestCase.java.
This one is in the right neighborhood, but it's not a perfect fit. If
you wanted to be a "Done, and Gets Things Smart" [1] person, you could
make a new java package for tests that deal with
ProjectStage==Development features and put NavigationTestCase and your
new test case in there. However, that's you're call. I'm not going
to ask you to do that.
In any case, I suggest using your IDE's refactor copy feature to copy
NavigationTestCase to MissingFormTestCase.
2. make a new xhtml file. Looking at
NavigationTestCase.testNullOutcomeNoMessages(), it points to an xhtml
file. Again, it's not a perfect fit, and you could refactor and make
a new diretory, etc. I suggest copying the xhtml file mentioned in
that test method to a new place,
jsf-ri/systest/web/facelets/missingForm.xhtml, and hacking away all
the markup and replacing it with some markup that would cause your
error message to be displayed.
3. Hack your new MissingFormTestCase and replace it with the necessary
content to assert that the correct message is displayed.
SECTION: Review on the production code portion of the change bundle
M jsf-ri/src/main/resources/com/sun/faces/resources/Messages.properties
+com.sun.faces.FORM_OMITTED_FOR_ACTIONSOURCE=The button/link component
needs to have a Form in its ancestry. Please add <h:form>.
- It looks like this error message *and* symbolic constant was not
updated when your understanding of the bug-fix was updated to reflect
the desired intent.
- When you go about adding a message like this, you have to ensure it's
localizable. Please take a look at the diffs of Andy's recent commits
regarding issue 1800 to see how to do this. svn log and svn diff are
your friend here. svn-book.pdf is definitely also your friend.
M jsf-ri/src/main/java/com/sun/faces/facelets/tag/jsf/ComponentSupport.java
- + private static boolean inspectInterfaces(Class[] interfaces) {
The comment on the method is accurate, but the method name is not.
Please rename to a more descriptive name.
- + private static boolean inspectParentAncestry(UIComponent
component) {
Same here.
M jsf-ri/src/main/java/com/sun/faces/util/MessageUtils.java
- It looks like you missed this comment
// IMPORTANT - ensure that any new message constant is properly
// tested in test/com/sun/faces/util/TestUtil_messages (see comments
// in test class for details on the test).
No problem, it's easy to miss.
[1]
http://steve-yegge.blogspot.com/2008/06/done-and-gets-things-smart.html
--
| ed.burns_at_sun.com | office: +1 407 458 0017
| homepage: | http://ridingthecrest.com/
| 5 work days until JSF 2.1 Milestone 5