Thanks for the update. I think the webrev is ready for putback.
Best,
Joe
On 1/7/2015 9:41 PM, Tristan Yan wrote:
Hi Joe/Lance and others.
Please review updated version for this one.
http://cr.openjdk.java.net/~tyan/8051563/webrev.01/
<http://cr.openjdk.java.net/%7Etyan/8051563/webrev.01/>
Thank you
Tristan
On Jan 6, 2015, at 2:27 PM, huizhe wang <huizhe.w...@oracle.com
<mailto:huizhe.w...@oracle.com>> wrote:
On 1/6/2015 2:25 PM, Lance Andersen wrote:
One more thing :-)
Remember to update your copyright year to 2015 also
Indeed, that applies to my webrevs as well :-)
Joe
Best
Lance
On Jan 6, 2015, at 5:04 PM, Lance Andersen
<lance.ander...@oracle.com <mailto:lance.ander...@oracle.com>> wrote:
On Jan 6, 2015, at 4:31 PM, Tristan Yan <tristan....@oracle.com
<mailto:tristan....@oracle.com>> wrote:
Thank you Lance and Joe.
You are very welcome.
I am working on the fix.
No rush from my perspective, have a lot to keep me busy in the
interim before your next webrev . :-)
it may take a couple of days that the code needs some refactor.
Understand, as I have been working on tests for RowSets, I have
continued to play the refactor dance myself.
Best,
Lance
I will send out next review once I finish it.
Thanks
Tristan
On Jan 6, 2015, at 1:22 PM, huizhe wang <huizhe.w...@oracle.com
<mailto:huizhe.w...@oracle.com>> wrote:
Thanks for taking the initiative and effort to refactor and clean
up all of the Functional tests in previous changeset!
We've gone through several iterations for the new ones (xslt
tests). I totally agree with Lance, it looks very good overall, a
lot better than its original form.
It's nice to group all of the tests that required file access,
that prepares all other tests to run with minimal permission for
a (future) secure-test-target. Unit tests might need a similar
treatment, but no pressure to do now :-)
A minor comment on utility classes: there's JAXPTestUtilities and
TestUtils. The former is an util for all tests while the later
contained a couple of SAX handlers, it may make sense to move
them into their own classes just as the other Handlers. The
constants (XML_DIR and GOLDEN_DIR) then can be declared in a base
class for the SAX tests.
I understand each group of tests have their own XML source and
golden files, thus XML_DIR and GOLDEN_DIR. It may be possible to
add a base for each group while put test.src and test.classes
into the very base class for all tests. So in general, we would have:
TestBase FileTestBase
Base for each group (e.g. SAXTestBase, DOMTestBase,
XSLTTestBase...) that extends either TestBase or FileTestBase
I remember there were a few tests that required a http server. So
then we may need a HttpTestBase as well.
I know this is not what we've discussed (or planned) previously.
But since you've done a great job to incorporate what were
previously quite separate test suites into one whole test suite.
I can't resist to ask a bit more. Don't get me wrong, what you've
done exceeded my expectation in a big way (only a month ago, we
were still talking about quick/straight conversion so that you
can start to take care of the new features)!
SAXParserTest: I noticed Old: testParse11, testParse27 --> New:
testParse05. So is testParse03 a new test? I can see
SAXException is expected, but not IOE. In fact, this shows that
the JAXP spec missed defining how empty string shall be handled
(should have been an IAE).
Best,
Joe
On 1/6/2015 11:33 AM, Lance Andersen wrote:
Hi Tristan,
Sorry for the delay but with the holidays and the number of
files, it took a while to go through :-)
Overall, it looks pretty good.
A couple of suggestions, but I would not necessarily hold back
from committing:
- For tests where you are not caring about the actual exception
that is thrown to indicate a failure, such as
in DocumentBuilderFactory01.java and testCheckSchemaSupport1, I
would just have the method declaration use "throws Exception"
vs list all of the possible individual Exceptions, as it
keeps it more compact. Glad to see you are not using
failUnexcepted() now.
- In test classes such as in testCheckSchemaSupport3. java
and DocumentBuilderImpl01.java, some tests do not use assertXXX
or expect an Exception. Would be good at least to document what
could cause a failure or make it clear the expected behavior of
the test for passing.
-SAXParserTest02.java and other tests where you get a
reader/parser such as testXMLReader01, I would at least assert
that null is not returned where as it is now, you only validate
that an exception is not returned
- I know you are porting existing tests, but I would consider
consolidating tests as it seems like there is not a real reason
to have a testNG class with just 1 test. I would group the
like tests (such as SAXTFactoryTest ) in one testNG test class
as that allows you to streamline further.
- TfClearParamTest.java (as and example) minor nit that you have
your @Before/AfterGroups method in between tests. I would
suggest grouping all methods such as this DataProviders before
or after the actual tests for better organization
- TraxSAXWrapper.java, not sure I understand the "Sorry I could
not resist comment"
- TransformerHandlerAPITest.java has typos in comments:
"IllegalArgumentExceptionis thrown…."
- Minitest.java, I would add a comment for your Data Provider
Best,
Lance
On Jan 2, 2015, at 1:49 PM, Tristan Yan <tristan....@oracle.com
<mailto:tristan....@oracle.com>> wrote:
Hi Joe and Lance
Sorry for my late reply. I just uploaded the new webrev which
is trying to limit minimal permissions for most of tests. The
new changeset has two base classes; class JAXPBaseTest has only
minimal set permissions; class JAXPFileBaseTest adds two
permissions that need access local files in the directory
directory test.src and test.classes. Most of tests only
inherit JAXPBaseTest that provides only minimal set
permissions. Some tests will inherit JAXPFileBaseTest because
tests need access local files.
Please help to review the code.
http://cr.openjdk.java.net/~tyan/8051563/webrev.00/
<http://cr.openjdk.java.net/%7Etyan/8051563/webrev.00/>
Thank you
Tristan
On Jan 2, 2015, at 10:39 AM, huizhe wang
<huizhe.w...@oracle.com <mailto:huizhe.w...@oracle.com>> wrote:
Lance,
Tristan is looking into adding an extension base class for
about 60 tests that require file permission, then the current
base class would indeed set "minimal" permission. So please
wait for his update :-)
Best,
Joe
On 12/30/2014 3:07 PM, Lance @ Oracle wrote:
Hi Tristan,
I will look at this but doubt I will get to this tomorrow
Best,
Lance
<http://oracle.com/us/design/oracle-email-sig-198324.gif>Lance Andersen|
Principal Member of Technical Staff | +1.781.442.2037
<tel:+1.781.442.2037>
Oracle Java Engineering
1 Network Drive <x-apple-data-detectors://34/0>
Burlington, MA 01803 <x-apple-data-detectors://34/0>
lance.ander...@oracle.com <mailto:lance.ander...@oracle.com>
Sent from my iPad
On Dec 30, 2014, at 5:28 PM, Tristan Yan
<tristan....@oracle.com <mailto:tristan....@oracle.com>> wrote:
Hi All
Can I get your review on this change.
http://cr.openjdk.java.net/~tyan/8051563/webrev.00/
<http://cr.openjdk.java.net/%7Etyan/8051563/webrev.00/>
<http://cr.openjdk.java.net/~tyan/8051563/webrev.00/
<http://cr.openjdk.java.net/%7Etyan/8051563/webrev.00/>>
These fixes originally come from bug
https://bugs.openjdk.java.net/browse/JDK-8051563
<https://bugs.openjdk.java.net/browse/JDK-8051563> as part
of our XML test colocation work. ThIS change-set mainly
covers tests for package org.apache.qetest.dtm and
org.apache.qetest.trax.
In the meantime I took steps at fixing some of our existed
test code on below issues:
1. Add a base test class for all functional tests that
enable security manager running. A limited minimal
permissions set have been set for every test.
2. Remove all unnecessary exception capture for functional
tests that we’re using testng to handle all the exceptions.
3. Use try-resource block to solve all possible resource
leaks (including InputStream, OutputStream, Writer, Reader).
Thanks a lot.
Tristan
<Mail Attachment.gif>
<http://oracle.com/us/design/oracle-email-sig-198324.gif>
<http://oracle.com/us/design/oracle-email-sig-198324.gif><http://oracle.com/us/design/oracle-email-sig-198324.gif>
<http://oracle.com/us/design/oracle-email-sig-198324.gif>Lance
Andersen| Principal Member of Technical Staff | +1.781.442.2037
Oracle Java Engineering
1 Network Drive
Burlington, MA 01803
lance.ander...@oracle.com <mailto:lance.ander...@oracle.com>
<oracle_sig_logo.gif>
<http://oracle.com/us/design/oracle-email-sig-198324.gif>
<http://oracle.com/us/design/oracle-email-sig-198324.gif><http://oracle.com/us/design/oracle-email-sig-198324.gif>
<http://oracle.com/us/design/oracle-email-sig-198324.gif>Lance
Andersen| Principal Member of Technical Staff | +1.781.442.2037
Oracle Java Engineering
1 Network Drive
Burlington, MA 01803
lance.ander...@oracle.com <mailto:lance.ander...@oracle.com>
<Mail Attachment.gif>
<http://oracle.com/us/design/oracle-email-sig-198324.gif>
<http://oracle.com/us/design/oracle-email-sig-198324.gif><http://oracle.com/us/design/oracle-email-sig-198324.gif>
<http://oracle.com/us/design/oracle-email-sig-198324.gif>Lance
Andersen| Principal Member of Technical Staff | +1.781.442.2037
Oracle Java Engineering
1 Network Drive
Burlington, MA 01803
lance.ander...@oracle.com <mailto:lance.ander...@oracle.com>
