Details

    • Type: Task Task
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: None
    • Component/s: core / taglib.core
    • Labels:
      None

      Description

      This includes a patch script which fixes the following JIRA entries:

      JELLY-184 Using namespace-prefixes breaks Jelly
      – Fixed, with unit test written
      JELLY-187 Wrong composite expression evaluation
      – Applied patch from JIRA and re-run unit tests
      JELLY-165 CatchTag closest from java tryCatch block (with expected exceptions list)
      – Applied patch and unit tests
      JELLY-163 Allow Expressions to throw exceptions
      – Wrote unit tests and modified code to allow JellyContext to enable/disable exceptions
      JELLY-13 Jelly should throw an exception if an unknown tag is used in a TagLibrary
      – Popular bug (5 votes for change) but would break backward compatability so a boolean flag is added to TagLibrary

      While it's slightly unusual to bunch patches in this way, it's done so that the existing JIRA database is not polluted by references between apparently unrelated issues, and so that the Commons team have a chance to evaluate my patches and provide a critique.

      1. JELLY-286-v2.patch
        24 kB
        John Spackman
      2. JELLY-286.patch.txt
        69 kB
        John Spackman

        Issue Links

          Activity

          Hide
          John Spackman added a comment -

          Patch script

          Show
          John Spackman added a comment - Patch script
          Hide
          John Spackman added a comment -

          Please note that in several cases, the patch in JELLY-286 includes patch scripts provided by others and where the original is attached to the linked issues. Patch JELLY-286 is to bring all these patches "under one roof" and to add unit tests (where necessary)

          Show
          John Spackman added a comment - Please note that in several cases, the patch in JELLY-286 includes patch scripts provided by others and where the original is attached to the linked issues. Patch JELLY-286 is to bring all these patches "under one roof" and to add unit tests (where necessary)
          Hide
          Paul Libbrecht added a comment -

          John,

          I guess I could try to fix it but:

          [javac] /Users/paul/data/projects/jakarta-commons/jelly/src/java/org/apache/commons/jelly/test/BaseJellyTest.java:43: cannot find symbol
          [javac] symbol : variable TJTagLibrary
          [javac] location: class org.apache.commons.jelly.test.BaseJellyTest
          [javac] context.registerTagLibrary(TJTagLibrary.NS, TJTagLibrary.class.getName());
          [javac] ^
          [javac] /Users/paul/data/projects/jakarta-commons/jelly/src/java/org/apache/commons/jelly/test/BaseJellyTest.java:43: cannot find symbol
          [javac] symbol : class TJTagLibrary
          [javac] location: class org.apache.commons.jelly.test.BaseJellyTest
          [javac] context.registerTagLibrary(TJTagLibrary.NS, TJTagLibrary.class.getName());
          [javac] ^

          do you really need that register in src directory?

          Try with maven clean jar.

          paul

          Show
          Paul Libbrecht added a comment - John, I guess I could try to fix it but: [javac] /Users/paul/data/projects/jakarta-commons/jelly/src/java/org/apache/commons/jelly/test/BaseJellyTest.java:43: cannot find symbol [javac] symbol : variable TJTagLibrary [javac] location: class org.apache.commons.jelly.test.BaseJellyTest [javac] context.registerTagLibrary(TJTagLibrary.NS, TJTagLibrary.class.getName()); [javac] ^ [javac] /Users/paul/data/projects/jakarta-commons/jelly/src/java/org/apache/commons/jelly/test/BaseJellyTest.java:43: cannot find symbol [javac] symbol : class TJTagLibrary [javac] location: class org.apache.commons.jelly.test.BaseJellyTest [javac] context.registerTagLibrary(TJTagLibrary.NS, TJTagLibrary.class.getName()); [javac] ^ do you really need that register in src directory? Try with maven clean jar. paul
          Hide
          Paul Libbrecht added a comment -

          Also, I'm fearing that 285 is included or supposed here, can it be?

          paul

          Show
          Paul Libbrecht added a comment - Also, I'm fearing that 285 is included or supposed here, can it be? paul
          Hide
          Paul Libbrecht added a comment -

          I have now committed the contents of this patch.

          But the following tests still failed:

          • TestNamespacePrefixes
          • TestNestedExceptions
          • TestUnknownTags
          • TestCustomExpressionFactory

          and were excluded in the project.xml.

          I also had to remove the reference to the TJTagLibrary from TagLibrary.java as well as the reference to the jelly define tag-library from some core-unit-tests (maybe this one should be allowed but it doesn't fit well).

          thanks to provide a subsequent patch to the current svn.

          paul

          Show
          Paul Libbrecht added a comment - I have now committed the contents of this patch. But the following tests still failed: TestNamespacePrefixes TestNestedExceptions TestUnknownTags TestCustomExpressionFactory and were excluded in the project.xml. I also had to remove the reference to the TJTagLibrary from TagLibrary.java as well as the reference to the jelly define tag-library from some core-unit-tests (maybe this one should be allowed but it doesn't fit well). thanks to provide a subsequent patch to the current svn. paul
          Hide
          John Spackman added a comment -

          Hi Paul,

          Attached is a new path which fixes those errors for ant and maven.

          John

          Show
          John Spackman added a comment - Hi Paul, Attached is a new path which fixes those errors for ant and maven. John
          Hide
          Paul Libbrecht added a comment -

          Patch 286-v2.patch applied.

          Had troubles with BaseJellyTest which seems to be in both places and should only be in src/java.

          After my patch I still have to disable the same tests.

          Sorry, am not too productive here!

          paul

          Show
          Paul Libbrecht added a comment - Patch 286-v2.patch applied. Had troubles with BaseJellyTest which seems to be in both places and should only be in src/java. After my patch I still have to disable the same tests. Sorry, am not too productive here! paul
          Hide
          John Spackman added a comment -

          BaseJellyTest should have been moved into src/test from src/java (sorry Paul, I had assumed that the patch would have picked that up). The BaseJellyTest class is only used by test code (I've checked Jelly's src/test and the tags) and by moving it into the src/test solves compilation problems.

          John

          Show
          John Spackman added a comment - BaseJellyTest should have been moved into src/test from src/java (sorry Paul, I had assumed that the patch would have picked that up). The BaseJellyTest class is only used by test code (I've checked Jelly's src/test and the tags) and by moving it into the src/test solves compilation problems. John
          Hide
          Paul Libbrecht added a comment -

          the problem is that test-code of tag-libs cannot be based on BaseJellyTest since test-code is not published code.

          So BaseJellyTest really should in src/java.

          Is this the thing that affects all the disabled tests? Could be... I find it bizarre.

          paul

          Show
          Paul Libbrecht added a comment - the problem is that test-code of tag-libs cannot be based on BaseJellyTest since test-code is not published code. So BaseJellyTest really should in src/java. Is this the thing that affects all the disabled tests? Could be... I find it bizarre. paul
          Hide
          John Spackman added a comment -

          Hi Paul,

          But none of the taglibs depend on BaseJellyTest (the swing taglib has its own).

          The problem was that BaseJellyTest has a custom JellyContext which uses the new test-only tag library, which is used to simulate events or generate data etc for running junit tests. This new test-only taglib should only be available for testing so I added to the JellyContext in BaseJellyTest. However, this means that BaseJellyTest will not compile when it is in src/java because it has a dependancy on files in src/test. I moved it into src/test from src/java because src/java seemed like the wrong place for it to be, and if third party taglibs depend on it they should have (and have no problem with) a dependency on Jelly's src/test; this has no impact on functionality and cleans up dependancies.

          John

          Show
          John Spackman added a comment - Hi Paul, But none of the taglibs depend on BaseJellyTest (the swing taglib has its own). The problem was that BaseJellyTest has a custom JellyContext which uses the new test-only tag library, which is used to simulate events or generate data etc for running junit tests. This new test-only taglib should only be available for testing so I added to the JellyContext in BaseJellyTest. However, this means that BaseJellyTest will not compile when it is in src/java because it has a dependancy on files in src/test. I moved it into src/test from src/java because src/java seemed like the wrong place for it to be, and if third party taglibs depend on it they should have (and have no problem with) a dependency on Jelly's src/test; this has no impact on functionality and cleans up dependancies. John
          Hide
          Paul Libbrecht added a comment -

          No, BaseJellyTest is imported in swing taglib and I think this is rather good practice.
          Also... remember.... backwards compatibility, don't remove things.

          I would suggest you make a "TJBaseJellyTest". How doable does it sound?

          paul

          Show
          Paul Libbrecht added a comment - No, BaseJellyTest is imported in swing taglib and I think this is rather good practice. Also... remember.... backwards compatibility, don't remove things. I would suggest you make a "TJBaseJellyTest". How doable does it sound? paul
          Hide
          Paul Libbrecht added a comment -

          John,

          I finally managed to simply add addCustomTagLib to BaseJellyTest which is overridden in your test-cases and makes them work. They are now removed from exclusion. I dare thus close this issue!

          Can you close the linked issues?

          thanks

          paul

          Show
          Paul Libbrecht added a comment - John, I finally managed to simply add addCustomTagLib to BaseJellyTest which is overridden in your test-cases and makes them work. They are now removed from exclusion. I dare thus close this issue! Can you close the linked issues? thanks paul

            People

            • Assignee:
              Paul Libbrecht
              Reporter:
              John Spackman
            • Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development