Harmony
  1. Harmony
  2. HARMONY-6608

[classlib][archive]Unit tests to improve the coverage of archive module

    Details

    • Type: Test Test
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 5.0M15
    • Component/s: Classlib
    • Labels:
      None
    • Environment:
      Linux and Windows
    • Patch Info:
      Patch Available

      Description

      Added units-tests which increases the test-coverage of archive module.

      1. 001_HARMONY-6608.patch
        17 kB
        Mohanraj Loganathan
      2. 002_HARMONY-6608.patch
        5 kB
        Mohanraj Loganathan
      3. 003_HARMONY-6608_Java6.patch
        22 kB
        Mohanraj Loganathan
      4. 004_HARMONY-6608_Java5.patch
        21 kB
        Mohanraj Loganathan

        Activity

        Hide
        Mohanraj Loganathan added a comment -

        Few more tests added

        Show
        Mohanraj Loganathan added a comment - Few more tests added
        Hide
        Mark Hindess added a comment -

        Thanks for the patches. I wonder about tests like:

        + try

        { + Deflater defl = new Deflater(); + defl.setLevel(2); + outPutBuf = new byte[500]; + defl.setInput(byteArray); + defl.setLevel(3); + fail("IllegalStateException expected"); + }

        catch(IllegalStateException ise)

        { + //expected + }

        and whether they should really be written as:

        + Deflater defl = new Deflater();
        + defl.setLevel(2);
        + outPutBuf = new byte[500];
        + defl.setInput(byteArray);
        + try{ + defl.setLevel(3); + fail("IllegalStateException expected"); + }catch(IllegalStateException ise){ + //expected + }

        since it is really only the exception from the second setLevel call that should cause the test to pass? There are several tests in your patch that have this problem.

        Also, it would be good to describe in the comments how you intend the patches to be applied. In particular, both patches appear to be valid for java5 except for the DeflaterInputStreamTest which is java6 only. Perhaps it would be better to supply separate patches for the tests that are for java5 trunk and those that are only applicable to the java6 branch?

        Show
        Mark Hindess added a comment - Thanks for the patches. I wonder about tests like: + try { + Deflater defl = new Deflater(); + defl.setLevel(2); + outPutBuf = new byte[500]; + defl.setInput(byteArray); + defl.setLevel(3); + fail("IllegalStateException expected"); + } catch(IllegalStateException ise) { + //expected + } and whether they should really be written as: + Deflater defl = new Deflater(); + defl.setLevel(2); + outPutBuf = new byte [500] ; + defl.setInput(byteArray); + try{ + defl.setLevel(3); + fail("IllegalStateException expected"); + }catch(IllegalStateException ise){ + //expected + } since it is really only the exception from the second setLevel call that should cause the test to pass? There are several tests in your patch that have this problem. Also, it would be good to describe in the comments how you intend the patches to be applied. In particular, both patches appear to be valid for java5 except for the DeflaterInputStreamTest which is java6 only. Perhaps it would be better to supply separate patches for the tests that are for java5 trunk and those that are only applicable to the java6 branch?
        Hide
        Mark Hindess added a comment -

        Also, tests like:

        + /**
        + * @tests java.util.jar.JarException#JarException(java.lang.String)
        + */
        + public void test_ConstructorLjava_lang_String1() throws Exception {
        + try

        { + throw new JarException("Jar Exception"); + }

        catch (JarException e)

        { + assertEquals("Jar Exception", e.getMessage()); + }

        + }
        +
        + /**
        + * @tests java.util.jar.JarException#JarException()
        + */
        + public void test_Constructor_void() throws Exception {
        + try

        { + throw new JarException(); + }

        catch (JarException e)

        { + // Correct + }

        + }

        should probably just be written:

        + /**
        + * @tests java.util.jar.JarException#JarException(java.lang.String)
        + */
        + public void test_ConstructorLjava_lang_String1() throws Exception

        { + assertEquals("Jar Exception", new JarException("Jar Exception").getMessage()); + }


        +
        + /**
        + * @tests java.util.jar.JarException#JarException()
        + */
        + public void test_Constructor_void() throws Exception

        { + new JarException(); + }

        since try/catch is already exercised sufficiently by other tests. Having said that, I'd question the value of these tests since the implementations of the tested constructors are only calls to super(...). If I was working on improving test coverage then I suspect these would be very low on my list of priorities since they seem rather unlikely to fail.

        Show
        Mark Hindess added a comment - Also, tests like: + /** + * @tests java.util.jar.JarException#JarException(java.lang.String) + */ + public void test_ConstructorLjava_lang_String1() throws Exception { + try { + throw new JarException("Jar Exception"); + } catch (JarException e) { + assertEquals("Jar Exception", e.getMessage()); + } + } + + /** + * @tests java.util.jar.JarException#JarException() + */ + public void test_Constructor_void() throws Exception { + try { + throw new JarException(); + } catch (JarException e) { + // Correct + } + } should probably just be written: + /** + * @tests java.util.jar.JarException#JarException(java.lang.String) + */ + public void test_ConstructorLjava_lang_String1() throws Exception { + assertEquals("Jar Exception", new JarException("Jar Exception").getMessage()); + } + + /** + * @tests java.util.jar.JarException#JarException() + */ + public void test_Constructor_void() throws Exception { + new JarException(); + } since try/catch is already exercised sufficiently by other tests. Having said that, I'd question the value of these tests since the implementations of the tested constructors are only calls to super(...). If I was working on improving test coverage then I suspect these would be very low on my list of priorities since they seem rather unlikely to fail.
        Hide
        Mohanraj Loganathan added a comment -

        Thanks for the comments Mark. I will work on your comments and submit a new patch for the same.

        Attached patch is only applicable for Java6 level. I will submit patch for Java5 level also.

        Show
        Mohanraj Loganathan added a comment - Thanks for the comments Mark. I will work on your comments and submit a new patch for the same. Attached patch is only applicable for Java6 level. I will submit patch for Java5 level also.
        Hide
        Mohanraj Loganathan added a comment -

        Attaching the patch for java6 and java5 branch with the implementation of above comments.

        003_HARMONY-6608_Java6.patch - To be applied under java6 branch
        004_HARMONY-6608_Java5.patch - To be applied under java5 branch

        Thanks and Regards,
        Mohan

        Show
        Mohanraj Loganathan added a comment - Attaching the patch for java6 and java5 branch with the implementation of above comments. 003_ HARMONY-6608 _Java6.patch - To be applied under java6 branch 004_ HARMONY-6608 _Java5.patch - To be applied under java5 branch Thanks and Regards, Mohan
        Hide
        Mark Hindess added a comment -

        I applied the java5 patch at r984577, but I removed the two tests for "IllegalStateException expected" that the patch would add to DeflaterTest as these tests failed on the RI. Did you
        test the tests against the RI. Would you expect these tests to fail?

        Show
        Mark Hindess added a comment - I applied the java5 patch at r984577, but I removed the two tests for "IllegalStateException expected" that the patch would add to DeflaterTest as these tests failed on the RI. Did you test the tests against the RI. Would you expect these tests to fail?
        Hide
        Mark Hindess added a comment -

        I've merged the changes from java5 to java6. I was going to apply the java6 patch. However, it
        is all included in the merge of the java5 changes with the exception of the DeflaterTest case discussed above and a (trivial) DataFormatException test that is:

        1) not java6 specific

        2) probably more reasonably belongs in a new DataFormatExceptionTest.java test file

        Show
        Mark Hindess added a comment - I've merged the changes from java5 to java6. I was going to apply the java6 patch. However, it is all included in the merge of the java5 changes with the exception of the DeflaterTest case discussed above and a (trivial) DataFormatException test that is: 1) not java6 specific 2) probably more reasonably belongs in a new DataFormatExceptionTest.java test file
        Hide
        Mark Hindess added a comment -

        I've merged the changes from java5 to java6. I was going to apply the java6 patch. However, it
        is all included in the merge of the java5 changes with the exception of the DeflaterTest case discussed above and a (trivial) DataFormatException test that is:

        1) not java6 specific

        2) probably more reasonably belongs in a new DataFormatExceptionTest.java test file

        Show
        Mark Hindess added a comment - I've merged the changes from java5 to java6. I was going to apply the java6 patch. However, it is all included in the merge of the java5 changes with the exception of the DeflaterTest case discussed above and a (trivial) DataFormatException test that is: 1) not java6 specific 2) probably more reasonably belongs in a new DataFormatExceptionTest.java test file
        Hide
        Hudson added a comment -

        Integrated in Harmony-1.5-head-linux-x86_64 #925 (See http://hudson.zones.apache.org/hudson/job/Harmony-1.5-head-linux-x86_64/925/)
        Apply modified version of java5 patch from "HARMONY-6608
        [classlib][archive]Unit tests to improve the coverage of archive module".

        I removed two tests that don't pass on the RI. They were the tests for
        "IllegalStateException expected" that the patch would add to DeflaterTest.
        correct behaviour should be.

        Show
        Hudson added a comment - Integrated in Harmony-1.5-head-linux-x86_64 #925 (See http://hudson.zones.apache.org/hudson/job/Harmony-1.5-head-linux-x86_64/925/ ) Apply modified version of java5 patch from " HARMONY-6608 [classlib] [archive] Unit tests to improve the coverage of archive module". I removed two tests that don't pass on the RI. They were the tests for "IllegalStateException expected" that the patch would add to DeflaterTest. correct behaviour should be.
        Hide
        Hudson added a comment -

        Integrated in Harmony-select-1.5-head-linux-x86_64 #90 (See http://hudson.zones.apache.org/hudson/job/Harmony-select-1.5-head-linux-x86_64/90/)
        Apply modified version of java5 patch from "HARMONY-6608
        [classlib][archive]Unit tests to improve the coverage of archive module".

        I removed two tests that don't pass on the RI. They were the tests for
        "IllegalStateException expected" that the patch would add to DeflaterTest.
        correct behaviour should be.

        Show
        Hudson added a comment - Integrated in Harmony-select-1.5-head-linux-x86_64 #90 (See http://hudson.zones.apache.org/hudson/job/Harmony-select-1.5-head-linux-x86_64/90/ ) Apply modified version of java5 patch from " HARMONY-6608 [classlib] [archive] Unit tests to improve the coverage of archive module". I removed two tests that don't pass on the RI. They were the tests for "IllegalStateException expected" that the patch would add to DeflaterTest. correct behaviour should be.
        Hide
        Mohanraj Loganathan added a comment -

        I didnt test this patch in RI. I am not sure how RI Handles the setLevel and setStrategy functions when the inputBuffer of deflater is not null.

        I will investigate it further and raise JIRA for this if its a bug in Harmony.

        I verified the commits. Except the 2 IllegalStateException exception expected tests in DeflaterTest, other changes are applied as expected both in java5 and java6

        Show
        Mohanraj Loganathan added a comment - I didnt test this patch in RI. I am not sure how RI Handles the setLevel and setStrategy functions when the inputBuffer of deflater is not null. I will investigate it further and raise JIRA for this if its a bug in Harmony. I verified the commits. Except the 2 IllegalStateException exception expected tests in DeflaterTest, other changes are applied as expected both in java5 and java6
        Hide
        Mohanraj Loganathan added a comment -

        Raised a separate JIRA for the trivial tests which are expecting IllegalStateException But RI didnt.

        https://issues.apache.org/jira/browse/HARMONY-6623

        Show
        Mohanraj Loganathan added a comment - Raised a separate JIRA for the trivial tests which are expecting IllegalStateException But RI didnt. https://issues.apache.org/jira/browse/HARMONY-6623
        Hide
        Mark Hindess added a comment -

        Resolved and verified. (Remaining work split to a separate JIRA.)

        Show
        Mark Hindess added a comment - Resolved and verified. (Remaining work split to a separate JIRA.)

          People

          • Assignee:
            Mark Hindess
            Reporter:
            Mohanraj Loganathan
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development