Uploaded image for project: 'JDK'
  1. JDK
  2. JDK-8264454

Jaxp unit test from open jdk needs to be improved

XMLWordPrintable

    • Icon: Bug Bug
    • Resolution: Fixed
    • Icon: P4 P4
    • 17
    • None
    • xml
    • None
    • b18

      Need to make the following common improvements:
      Should restore the environment at the end of test method to prevent impacting other test methods ot other tests
      Should add singleThreaded=true if global state is changed in test
      Need to close the resource e.g. file handle, XMLStreamReader, other Closeable and so on

      use exact assert, keep detail info in assert
      let the exception thrown from the test
      Make better naming
      try private as possible
      reduce duplicate code

      jaxp/test/javax/xml/jaxp/unittest/javax/xml/common/Bug6350682.java
      0. please use braces even for one-line block
      > if (Bug6350682.class.getClassLoader() == null)
      > System.out.println("this class loader is NULL");
      > else
      > System.out.println("this class loader is NOT NULL");
      1. use the most specific in catch clause.
      > } catch (Exception e) {
      > e.printStackTrace();
      > Assert.fail("Exception occured: " + e.getMessage());
      > }

      > } catch (Exception e) {
      > e.printStackTrace();
      > Assert.fail("Exception occured: " + e.getMessage());
      > } catch (TransformerFactoryConfigurationError error) {
      > error.printStackTrace();
      > Assert.fail(error.toString());
      > }

      2. no sure about testNG in jtreg, but for regular jtreg tests it doesn't make sense to print stack trace if you rethrow exception or pass it as cause to another.
      3. there is no needs to catch TransformerFactoryConfigurationError
      4. you change the thread environment, shouldn't you recover it in the end?

      jaxp/test/javax/xml/jaxp/unittest/javax/xml/common/Bug6723276Test.java
      0. can you make up better names than test0/test1?
      1. you change the global environment, shouldn't you recover it in the end?
      2. e.getMessage().indexOf("") > 0 sounds like contains 3. please introduce a string constant for "org.apache.xerces.jaxp.SAXParserFactoryImpl not found"
      4. these tests will pass, if any other exception are thrown. is it supposed?


      jaxp/test/javax/xml/jaxp/unittest/javax/xml/common/Bug6979306Test.java
      0. it's unclear that this test tests. please write more detailed summary

      jaxp/test/javax/xml/jaxp/unittest/javax/xml/common/Bug7143711Test.java
      0. SCHEMA_LANGUAGE,SCHEMA_SOURCE,ORACLE_FEATURE_SERVICE_MECHANISM should be private 1. ORACLE_FEATURE_SERVICE_MECHANISM should be static 2. lots of code duplication, the method which contains common part should be introduced 3. this test sounds very similar to Bug6979306Test. it's needed to extract common parts

      jaxp/test/javax/xml/jaxp/unittest/javax/xml/datatype/Bug6320118.java
      0. there is no needs to catch IllegalArgumentException in test{1,2,4} 1. this should be done in ctor or in "setUp" method
      > try {
      > df = DatatypeFactory.newInstance();
      > } catch (DatatypeConfigurationException e) {
      > Assert.fail(e.getMessage());
      > }
      2.
      > if (!c1.equals(c2))
      > Assert.fail("hour 24 needs to be treated as equal to hour
      > 0
      of the next day");
      Assert.assertEquals
      3. it's better to write this as two separate asserts
      > if (c1.getYear() != 2000 && c1.getHour() != 0)
      > Assert.fail("hour 24 needs to be treated as equal to hour
      > 0
      of the next day");

      jaxp/test/javax/xml/jaxp/unittest/javax/xml/datatype/Bug6937964Test.java
      0. 'fields' should be private static and upper cased 1. method test is a bad copy of testNewDurationYearMonthLexicalRepresentation
      2. Assert.assertEquals(years, 21,
      > Assert.assertTrue(years == 21,
      3. why do you need testNewDurationYearMonthLexicalRepresentation1 to be final?
      4. testNewDurationYearMonthLexicalRepresentation1 and
      testNewDurationDayTime005 are in a really mess and have to be rewritten



      *:
      3. it'd be better to provide an informal error message in all asserts
      4. it'd be better to pass exception to Assert.fail in order not to miss useful information about an error
      5. you don't close XMLStreamReader (and maybe some other closeable objects)
      IssueTracker24/Bug.java:
      0. > Assert.assertTrue(r.getPrefix() == ""); why do you check string identity? I think you need to check equality.
      AFAIR, testng provides assertEquals for that
       IssueTracker30/Bug.java
      0. please introduce a method, which gets xsd schema-file name and do all the stuff, and use this method in both test methods.
      1. I'd prefer to have 4 tests : {ok, error} x {occurs.xsd, occurs-optimize.xsd}
      2.
      > public void fatalError(SAXParseException e) throws SAXException {
      > System.out.println("Fatal error: " + e.getMessage()); }
      why do you think that it's ok to ignore fatal errors? I think we should fail the test if any.
      3.
      > public void error(SAXParseException e) throws SAXException {
      > System.out.println("Error: " + e.getMessage());
      > errorFound = true;
      > }
      is it possible to check that we get the error which we expect to get?

      IssueTracker35/Bug.java
      0. > Assert.assertTrue(e == XMLStreamConstants.DTD);
      > Assert.assertTrue(reader.getLocalName().equals("schema"));
      Assert.assertEquals?
      1.
      > while ((e = reader.next()) == XMLStreamConstants.COMMENT)
      > ;
      please add brackets
      2.
      > XMLStreamReader reader =
      xif.createXMLStreamReader(getClass().getResource("XMLSchema.xsd").getFile(
      ), getClass().getResourceAsStream("XMLSchema.xsd"));
      please introduce a local variable to store #getResource result

      IssueTracker38/Bug.java
      0.
      > try {
      > XMLEventReader xer = xIF.createXMLEventReader(source);
      > } catch (UnsupportedOperationException e) {
      > // e.printStackTrace();
      > try {
      > XMLStreamReader xsr = xIF.createXMLStreamReader(source);
      > } catch (UnsupportedOperationException oe) {
      > // e.printStackTrace();
      > return;
      > }
      > }
      > Assert.fail("Expected UnsupportedOperationException not
      thrown");

      I'd prefer
      try {
         XMLEventReader xer = xIF.createXMLEventReader(source);
         Assert.fail(...);
      } catch {
      }
      try {
         XMLStreamReader xsr = xIF.createXMLStreamReader(source);
         Assert.fail(...);
      } catch {
      }
      1. please introduce a method which gets j.x.t.Source as an argument and does all checks and use this method in both tests.
      2. I think it'd be better to have 4 tests {event,stream} x {dom, sax}

      IssueTracker70/Bug.java
      0. > File testFile = new
      File(getClass().getResource("test.xml").getFile());
      it should be private static final
      1.
      > public void testGetAttributeValueWithEmptyNs() throws Exception {
      > File testFile = new
      File(getClass().getResource("test.xml").getFile());
      why don't you use testFile member here?
      2.
      > Assert.assertTrue(v != null);
      Assert.assertNotNull
      3. please introduce a method which does all stuff except the check and gets j.u.f.Consumer<XMLStreamReader> as an argument. then the tests will look like:
      nullNs(XMLStreamReader xsr) {
         v = xsr.getAttributeValue(null, "attribute2");
         Asserts.assertTrue(v != null);
      }
      emptyNs(XMLStreamReader xsr) {
         v = xsr.getAttributeValue("", "attribute1");
         Asserts.assertTrue(v != null);
      }

      testGetAttributeValueWithNullNs() {
         testGetAttributeValue(::nullNs);
      }

      testGetAttributeValueWitEmptyNs() {
         testGetAttributeValue(::emptyNs);
      }

      testGetAttributeValue(Consumer<XMLStreamReader> onStartElement) {
        ...
        xsr.next();
        if (xsr.isStartElement()) {
          onStartElement.accept(xsr);
        }
      }

            mchhipa Mahendra Chhipa
            mchhipa Mahendra Chhipa
            Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

              Created:
              Updated:
              Resolved: