Hi Laurent,

Looks great.  Some comments on the test:

- A couple of methods have "/**" even though the comment is just informational to the programmer and not representing an API. It looks like "/**" is the standard practice for the @test comment at the top of the class even though jtreg isn't spec'd to require it, but elsewhere it's just noise.

- In general tests shouldn't print out a lot of noise (see "verbosity" in http://openjdk.java.net/jtreg/writetests.html). Ideally an automated test would produce no output if it is successful, but some of our automated tests do print out a short summary (mostly if they were stress-bashers). Some of them can be run in "verbose mode" to print out details in case an engineer is diagnosing a failure and wants to know what failed and why - I saw a few in the AffineTransform directory that had a static boolean verbose that was set if there was an argument handed to main() for instance. A verbose option can help when running the test in manual mode for debugging.

- I'd add a testAddCubic() and testAddClose() to the tests. It looks like the "testAddQuad" method does add both types of curve, but the cubic curve is added after the quad so we don't get a case that adds a cubic to a freshly cloned path - which is the main state in question to be tested here.

- equalsArray has some tests in it that don't make a lot of sense since you created the arrays and so you know that they aren't == or null or the wrong length. I'd rather see a "number of coords" parameter added to the method, get rid of the ==/null/length tests and only test the number of coords that were appropriate to the segment type. Technically you are testing entries in the arrays that weren't required to be equal by the PathIterator contract, but in practice it would be odd for the iterator to scribble random values into the unused entries (not disallowed, but odd).

- What am I missing on the difference between the two test files? Can they be combined into a single test file?

                        ...jim

On 3/27/15 9:33 AM, Laurent Bourgès wrote:
Jim,

Here is a new Path2d patch:
http://cr.openjdk.java.net/~lbourges/path2D/Path2D.2/

Changes:
- Fix ArrayIndexOutOfBounds in ptCrossing / rectCrossing when coords
array is zero-sized (0 capacity constructor): bug present in jdk8
- new tests:
Path2dEmpty to test this bug
Path2dCopyConstructor to test my changes on the copy constructor
following your instructions

Maybe we should create a new bug and split this patch in 2 separated ones ?

Laurent

Le 7 mars 2015 02:43, "Jim Graham" <james.gra...@oracle.com
<mailto:james.gra...@oracle.com>> a écrit :

First, the test cases that I asked for were to verify that a path that was 
cloned was still operational.  I see no tests that verify that the returned 
objects will still function, only tests that examine internal data structures 
for a metric that may not be appropriate in the future.

On 3/6/15 11:06 AM, Phil Race wrote:

So the test should go in the anonymous package and avoid accessing
internals.
It should be possible to use just public API  to verify the arrays  of a
shape
being cloned are trimmed .


We should not be testing if the cloned arrays are trimmed.  We don't care from a testing 
standpoint.  It is an implementation detail of a performance feature and we should not 
write any test that says "Object A should do its work by doing X, Y, and Z".

If we want to test performance then we should write a benchmark that tests how 
long it takes to do an operation.  That will likely be a manual test, though.  
It should not be examining internals to take its measurements, though.  
Benchmarks tend to only use public APIs as they should be benchmarking 
operations that Java programs encounter.

If we want to test if your fix will break something, then we need to test that. 
 Currently your tests do not do that.  We only need public APIs to do that.

What I would like to see in the test:

- Delete all of the existing tests in there.  None of them are appropriate to 
our testing goals.

- Write tests that create paths in various forms and run them through the 
following sub-tests:
     - each of the following should be tested on a fresh clone...
     - get a PathIterator and iterate it through until it is done
       (optional - compare to the expected segments that were in the original)
     - get a flattened PathIterator using "getPathIterator(flatness,)" and make 
sure it works
       (optional - compare to original path if the original was already flat)
       (but, also run it on a path with curves to make sure flattening engine 
didn't break)
     - add various kinds of segments to the cloned path
     - get the bounds of the cloned path
     - use the transform() method on the cloned path
     - call intersects(point), intersects(rect) and contains(rect) on a cloned 
path
     - call getCurrentPoint() on the cloned path

- Perform each of the above tests on a (set of) clone(s) of the following paths:
     - each of the following executed on both a Float and a Double instance...
     - an empty path
     - a path with just a moveto
     - a path with a moveto+some lines
     - a path with a moveto+some curves

The way I tend to write tests like this is to use cascading methods like:

void testEqual(Path2D pathA, Path2D path2D pathB) {
    // Grab 2 path iterators and test for equality with float coords[]
}

void test(Path2D p2d, boolean isEmpty) {
    testEqual(new Path2D.Float(p2d), p2d);
    testEqual(new Path2D.Double(p2d), p2d);
    testEqual(new GeneralPath(p2d), p2d);

    testIterator(new Path2D.Float(p2d));
    testIterator(new Path2D.Double(p2d));
    testIterator((Path2D) p2d.clone());

    testFlattening(... 3 clone variants ...);

    testAddMove(... 3 clone variants ...);
    // These should expect exception if empty
    testAddLine(... 3 clone variants ..., isEmpty);
    testAddQuad(... 3 clone variants ..., isEmpty);
}

interface PathFactory {
    Path2D makePath();
}

void test(PathFactory pf) {
    test(pf.makePath(), true);
    test(addMove(pf.makePath()), false);
    test(addLines(pf.makePath()), false);
    test(addCurves(pf.makePath())), false);
}

@Test
void testFloatPaths() {
    test(() -> new Path2D.Float());
}

@Test
void testDoublePaths() {
    test(() -> new Path2D.Double());
}

@Test
void testGeneralPath() {
    test(() -> new GeneralPath());
}

                        ...jim

Reply via email to