Re: seeking review 1663

From: Ed Burns <>
Date: Tue, 31 Aug 2010 07:35:42 -0700

>>>>> On Fri, 27 Aug 2010 10:34:06 -0700, Sheetal Vartak <> 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

  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/

+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/

- + 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/

- 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.

|        | office: +1 407 458 0017
| homepage:               |
|  5 work days until JSF 2.1 Milestone 5