Updates JMX statistics test to assert statistics update with a timeout

[vbekiaris]

Updates CacheMBStatisticsMBeanTest to assert:

• statistics updates are eventually observed within a timeout.
• in test cases where no statistics updates should occur, changes are not observed for the duration of the timeout.

The default timeout is 5 seconds and can be overridden via system property org.jsr107.tck.management.statistics.timeout.seconds.

Fixes #79

[cruftex]

Hi Vassilis, cool, thanks you take a shot on this. Actually, I would have done it myself or dropped it, since I reqeusted it.

There is an error in assertEventually, when the assertion isn't holding in the first place (no reset).
Its best to add unit tests for that itself :)

IMHO you can simplify to:

public static void assertEventually(AssertionRunnable assertionRunnable, int timeoutSeconds) throws Exception {
  long deadline = currentTimeMillis() + timeoutSeconds * 1000;
  while (currentTimeMillis() < deadline) {
    try {
      assertionRunnable.run();
      return;
    } catch (AssertionError assertionError) {
    }
    Thread.sleep(200);
  }
  assertionRunnable.run();
}

Major things:

• assertEventually needs to be corrected
• I think the tests with assertAllTheTime are not needed.

Minor things:

• maybe drop the throws Exception since AssertError is an unchecked exception
• maybe use an ojbect initialized with the timeout once new Xy(long, TimeUnit). So the call with the runnable is a single arg and displayed nicely as lambda in modern IDEs.

[cruftex]

Major things:

• assertEventually needs to be corrected
• I think the tests with assertAllTheTime are not needed.

Minor things:

• maybe drop the throws Exception since AssertError is an unchecked exception
• maybe use an ojbect initialized with the timeout once new Xy(long, TimeUnit). So the call with the runnable is a single arg and displayed nicely as lambda in modern IDEs.

[vbekiaris]

Thanks for the review Jens!

assertEventually needs to be corrected

To be honest I failed to see the error in the implementation, but maybe that's just due to lack of coffee on Monday morning :) . Nevertheless, I updated my PR with the simplified version you posted, looks much better.

I think the tests with assertAllTheTime are not needed.

I think it is needed to ensure the TCK catches errors in implementations that may delay publishing of statistics. For example, consider this:

// should not affect statistics
cache.containsKey(1);
assertEquals(0, lookupManagementAttribute(cache, CacheStatistics, "CacheHits"));

In a hypothetical implementation that updates published statistics every 10 seconds and erroneously increments CacheHits in containsKey, this test would fail to detect the error: it is required to test that for the configured duration of statistics update delay, CacheHits does not change.

maybe drop the throws Exception since AssertError is an unchecked exception

You are right, invokers of these methods should not be forced to handle checked exceptions. I have updated my PR so that any checked exceptions thrown from assertion code will now be wrapped in a RuntimeException.

maybe use an ojbect initialized with the timeout once new Xy(long, TimeUnit). So the call with the runnable is a single arg and displayed nicely as lambda in modern IDEs.

I have a slight preference towards keeping assertEventually & assertAllTheTime as static methods as I feel it enhances chances they might be useful in other tests, but I could also implement the change you suggest, no problem with that.

[cruftex]

In a hypothetical implementation that updates published statistics every 10 seconds and erroneously increments CacheHits in containsKey, this test would fail to detect the error: it is required to test that for the configured duration of statistics update delay, CacheHits does not change.

Gosh, I didn't think about that one. You are totally right, if an implementation delays then it wouldn't fail that test.

I thought we could have a reasonable high grace time, e.g. one minute, which should do the trick for all implementations. When a change is immediate the total runtime of the unit tests would not increase. However, if we add tests, that assert a value is not changing within a certain timeframe, then the runtime increases for everyone, or most implementations need to specify that the grace time is 0 seconds.

The best solution would be to make it testable e.g. by defining a flush instruction. But that would mean the Spec needs to be changed. The perfect solution needs to be postponed to 2.0...

Since the current tests don't cater for this problem, I have a slight tendency for ignoring it. Adding the delay and/or a parameter just seems to me quite a hassle. Another option would be to start a test suite of timing related tests, which has longer running times and eventually also addresses expiry.

[vbekiaris]

Another option would be to start a test suite of timing related tests, which has longer running times and eventually also addresses expiry.

I think that's a good strategy to deal with time-consuming tests. Other options may also include executing tests in parallel to lower test time, however these should be explored for 2.0.

That said, setting the default timeout to 0 seconds will not make a difference to current test time. Only implementations that actually defer statistics updates will need to set the timeout property to a suitable value and will have a longer test time, so I would be in favour of keeping this PR (assuming all other concerns are addressed).

[cruftex]

That said, setting the default timeout to 0 seconds will not make a difference to current test time. Only implementations that actually defer statistics updates will need to set the timeout property to a suitable value and will have a longer test time, so I would be in favour of keeping this PR

That's okay! Two little things:

// Default timeout while waiting for statistics to be updated is 5 seconds

5 seconds isn't true any more. Pls. make it a JavaDoc comment with /** .... */ to indicate it belongs to that field.

[vbekiaris]

Thanks @cruftex , javadoc was updated as suggested.

[cruftex]

Cool. Thanks again for taking care of this!