Re: RFC: JDWP filter tests

[Keith Seitz <keiths at redhat dot com>]

Kyle Galloway wrote:

> Index: gnu/testlet/gnu/classpath/jdwp/events/filters/TestOfClassExcludeFilter.java
> ===================================================================
> RCS file: gnu/testlet/gnu/classpath/jdwp/events/filters/TestOfClassExcludeFilter.java
> diff -N gnu/testlet/gnu/classpath/jdwp/events/filters/TestOfClassExcludeFilter.java
> --- /dev/null	1 Jan 1970 00:00:00 -0000
> +++ gnu/testlet/gnu/classpath/jdwp/events/filters/TestOfClassExcludeFilter.java	1 Jan 1970 00:00:00 -0000
> @@ -0,0 +1,97 @@

I only have one comment on this (and it applies to the others, too, 
probably): Are there any inputs to ClassMatchFilter which are simply 
illegal?

For example, the spec describes the pattern as "Disallowed class 
pattern. Matches are limited to exact matches of the given class pattern 
and matches of patterns that begin or end with '*'; for example, "*.Foo" 
or "java.*"." One of the errors that is listed at the end of the 
description of Event.Set is "INVALID_STRING".

Perhaps that is bit overboard, though. I don't really know.

You can't really do much with this for many filters, e.g., 
ClassOnlyFilter, because of they require valid IDs and the like, and 
they are checked by PacketProcessor and *CommandSet before creating the 
filter.

Of course, this begs the question: Should this check be done by the 
filter or the command set processor that is creating the filter? Alas, 
that's a different conversation for a different list. [The most prudent 
answer to this is that we should always test for invalid input. If that 
check is done in the code in the filter, then it should be tested in the 
filter. If the check is done somewhere else, it should be tested there, 
too. But regardless of where the check and test are located, they should 
both be done.]

> Index: gnu/testlet/gnu/classpath/jdwp/events/filters/TestOfClassOnlyFilter.java
> ===================================================================
> RCS file: gnu/testlet/gnu/classpath/jdwp/events/filters/TestOfClassOnlyFilter.java
> diff -N gnu/testlet/gnu/classpath/jdwp/events/filters/TestOfClassOnlyFilter.java
> --- /dev/null	1 Jan 1970 00:00:00 -0000
> +++ gnu/testlet/gnu/classpath/jdwp/events/filters/TestOfClassOnlyFilter.java	1 Jan 1970 00:00:00 -0000
> @@ -0,0 +1,105 @@
[snip]
> +  public void testConstructor(TestHarness harness)
> +  {
> +    harness.checkPoint("Constructor");
> +    for (int i = 0; i < 3; i++)
> +      {
> +        ReferenceTypeId testId = new ClassReferenceTypeId();
> +        SoftReference testSR = new SoftReference(Integer.class);
> +        testId.setReference(testSR);
> +
> +        try
> +          {
> +            ClassOnlyFilter testCOF = new ClassOnlyFilter(testId);
> +          }
> +        catch (InvalidClassException ex)
> +          {
> +            harness.check(false, "Constructor failed with exception" + ex);
> +          }
> +      }
> +  }

I think I mentioned this before, but you could simply use VMIdManager to 
get Object and ReferenceType IDs. No big deal, though, if you would 
rather avoid that.

What about testing the case where the constructor *should* fail, i.e., 
don't set the reference for the ID. [aside from the whole "should it be 
checked here somewhere else" thing again]

> Index: gnu/testlet/gnu/classpath/jdwp/events/filters/TestOfCountFilter.java
> ===================================================================
> RCS file: gnu/testlet/gnu/classpath/jdwp/events/filters/TestOfCountFilter.java
> diff -N gnu/testlet/gnu/classpath/jdwp/events/filters/TestOfCountFilter.java
> --- /dev/null	1 Jan 1970 00:00:00 -0000
> +++ gnu/testlet/gnu/classpath/jdwp/events/filters/TestOfCountFilter.java	1 Jan 1970 00:00:00 -0000
[snip]
> +    try
> +    {
> +      CountFilter testCF = new CountFilter(COUNT);
> +      for (int i = 0; i < COUNT; i++)
> +        {
> +          matched = testCF.matches(new ClassPrepareEvent(Thread.currentThread(),
> +                                                         Integer.class, 0));
> +          if(i < COUNT-1)
> +            harness.check(!matched, "Matches returns false for < count");
> +          else
> +            harness.check(matched, "Matches returns true after count # of"
> +                          + "events");
> +        }
> +    }
> +    catch(InvalidCountException ex)
> +    {
> +      harness.check(false, "Constructor failed for valid count");
> +    }
> +  }

I suggest looping over something like "COUNT + 1" to check what happens 
when the count has been exceeded. It should *only* match once. [Missing 
a space in the string "Matches returns true after count # ofevents".]

You could recycle the event instead of creating a new one every time. 
Probably doesn't matter, but it would be trivial enough in this case to 
keep that overhead out of the testsuite.

> Index: gnu/testlet/gnu/classpath/jdwp/events/filters/TestOfInstanceOnlyFilter.java
> ===================================================================
> RCS file: gnu/testlet/gnu/classpath/jdwp/events/filters/TestOfInstanceOnlyFilter.java
> diff -N gnu/testlet/gnu/classpath/jdwp/events/filters/TestOfInstanceOnlyFilter.java
> --- /dev/null	1 Jan 1970 00:00:00 -0000
> +++ gnu/testlet/gnu/classpath/jdwp/events/filters/TestOfInstanceOnlyFilter.java	1 Jan 1970 00:00:00 -0000
[snip]
> +    try
> +      {
> +        InstanceOnlyFilter testIOF = new InstanceOnlyFilter(testOID);
> +        harness.check(true, "Constructor successful");
> +      }
> +    catch (InvalidObjectException ex)
> +      {
> +        harness.check(false, "constructor failed for valid ObjectId");
> +      }
> +
> +  }

Another good place to try passing something invalid to the constructor 
and test whether it fails. Also, for this one, "null" should be tested 
as a constructor argument, since it is valid.

> +    try
> +      {
> +        InstanceOnlyFilter testIOF = new InstanceOnlyFilter(testOID);
> +        BreakpointEvent testEvent = new BreakpointEvent(Thread.currentThread(),
> +                                                        null, testInt);
> +        harness.check(testIOF.matches(testEvent), "testing Instance match");
> +      }
> +    catch (InvalidObjectException ex)
> +      {
> +        harness.check(false, "constructor failed for valid ObjectId");
> +      }
> +  }

Missing a test where this should fail.

Hmm. What about things to test object equality? Maybe add a test that 
checks against a clone of the object? I think for InstanceOnly, we need 
obj1 == obj2, not obj1.equals(obj2).

Keith