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);
}
}
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);
}
}