timing regression in the MF generator

[egonw]

@tomas-pluskal, can you please have a look at this regression?

I happens not in my local Eclipse but on Jenkins: https://jenkins.bigcat.unimaas.nl/job/cdk/lastCompletedBuild/testReport/org.openscience.cdk.formula/MolecularFormulaGeneratorTest/testCancel/

Do you see an option to ensure all required classes are loaded before the cancelling is tested?

[tomas-pluskal]

Not sure if there is a way to do that. Can we try to simply increase the timeout, e.g. to 1 s?
The test does not fail on my machine.

[johnmay]

Please do not do that. We have a mechanism for slow tests:

[johnmay]

Anything >~ 50 ms should be tagged with SlowTest.

@Test(timeout=1000)
@Category(SlowTest.class)

[tomas-pluskal]

Well, the test is not slow. The delay in the test is set to 5 ms. On my machine the test ends faster than 50 ms.

Setting the timeout to 1000 ms does not mean the test takes 1000 ms, it will still finish as soon as all the classes are loaded.

[egonw]

@johnmay, it's not a slow test. It finished in 38 ms on my machine. The problem is that on Jenkins, it seems to be loading some class, just tipping it over 100ms... that's not due to the test, but something in the VM...

[johnmay]

38 ms is slow :-)

[johnmay]

I'm not a fan off timeouts for this exact reason, it's better to bound on an iteration limit etc. Otherwise if someone has a really slow machine certain algorithms will fail whilst others will not.

[tomas-pluskal]

The test checks if a method running in one thread can be canceled from another thread. The cancellation is triggered in the second thread after 5 ms delay. I don't see how to implement this test using iterations - the timeout has a clear purpose here.

[egonw]

I don't quite get why the class loading is so slow when Jenkins runs the test... also, I am not sure which class is not loading fast enough, because if we know that, we can push that to the test class instantiation, i.e. the @BeforeClass method...

[johnmay]

@tomas-pluskal it's fine as it is - the iteration limit goes inside the algorithm but this wouldn't fix this use case. The CDK tests were in disarray a few years ago with many sloppy tests written, it took a lot of effort to get them passing and actually building in reasonable time. In general if a test can randomly fail (be it a timeout - or a random number generator, there is one of those in the CDK) it's not good. Extending the timeout is dirty, sure we'll probably never see the timeout again but maybe someone runs a JVM on a raspberry pi and the threading implementation is poor, maybe they will.