search

fangfangli / cleartk

Automatically exported from code.google.com/p/cleartk
0 stars 0 forks source link

Don't disable tests by default #239

Closed GoogleCodeExporter closed 9 years ago

GoogleCodeExporter commented 9 years ago 
Right now, by default, tests that require a long time or a lot of memory are 
disabled, and you have to specify -Dcleartk.longtests and -Dcleartk.bigmem to 
run them.

I think this is wrong. I think these tests should be enabled by default, and 
flags should only exist to disable them for users that choose to.

My main objection to the disabled-by-default approach is that there is way too 
much output from "mvn test" for anyone to notice the one little line that says 
"INFO: Skipping test because...". I even know where it's supposed to be and I 
had a hard time finding it.

So if someone checks out ClearTK, and runs "mvn test", they'll see "BUILD 
SUCCESS" at the bottom even though, for example, not a single real test on 
MaltParser was run. This is misleading.

Specifically, I propose that we replace the -Dcleartk.longtests and 
-Dcleartk.bigmem properties with -Dcleartk.skipTests=long,bigMem. See the 
attached patch.

I think this is a much better approach because if the tests hang or run out of 
memory, they do so immediately after printing a message like:

Mar 23, 2011 4:06:08 PM org.cleartk.syntax.dependency.clear.ClearParserTest 
testClearParser
INFO: This test requires a lot of memory. To skip it, set 
-Dcleartk.skipTests=bigMem

So you immediately know what the problem is and how to fix it, while with the 
current setup, you probably don't even realize there are tests being skipped 
because the messages scroll off the screen too fast.

Original issue reported on code.google.com by steven.b...@gmail.com on 23 Mar 2011 at 3:07

GoogleCodeExporter commented 9 years ago 
I think this makes sense.  This will be motivating to maybe refactor some of 
the really long running tests that test some of the different ML wrappers that 
I wrote a long time ago.  There was one that tests C45 decision trees that took 
20 minutes to run.  It should be possible to write something that doesn't take 
so long!

That said, as software projects grow the test suite generally grows to large to 
be run every time before code is committed.  Often, there is a subset of tests 
that represent a sanity check which is appropriate to run before committing 
code and then a complete set of tests which include regressions (which we 
haven't even begun to address) that may take hours to run and are generally run 
once a day by a continuous integration server which produces a daily report for 
the last night's build which decides who buys donuts for who.  

I think we could do something similar without hiring a QA team.  We could run 
the "sanity check" subset for commits and then run the whole thing prior to 
releases or when whim tells us to.

Original comment by phi...@ogren.info on 27 Mar 2011 at 4:00

GoogleCodeExporter commented 9 years ago 
Ok, I made this change in r2821.

Having a sanity check version sounds like a good idea to me. How about we 
redefine "long" to mean "runs for more than a second or two"? Then the sanity 
check can just be -Dcleartk.skipTests=long. You can put whatever you want into 
"long", but minimally it should probably include:

org.cleartk.syntax.opennlp.ParserAnnotatorTest (11.246 sec)
org.cleartk.syntax.berkeley.ParserAnnotatorTest (7.224 sec)
org.cleartk.syntax.dependency.clear.ClearParserTest (16.371 sec)
org.cleartk.syntax.dependency.malt.MaltParserTest (19.996 sec)
org.cleartk.stanford.StanfordCoreNLPTest (53.106 sec)
org.cleartk.timeml.TimeMLAnnotateTest (9.722 sec)
org.cleartk.examples.pos.ExamplePosClassifierTest (23.699 sec)
org.cleartk.examples.pos.NonSequenceExamplePOSAnnotatorTest (10.076 sec)

The times are on my computer where currently running with 
-Dcleartk.skipTests=long takes me 6m52.840 sec, so skipping the above would 
probably get me down around 4m. Still probably a little long for a sanity 
check. I'll let you decide what else to skip.

Original comment by steven.b...@gmail.com on 27 Mar 2011 at 4:36

GoogleCodeExporter commented 9 years ago 
I'm going to close this issue as the initial problem (tests being disabled by 
default) has now been fixed. If you'd like to open another issue to add more 
things to -Dcleartk.skipTests=long, please go ahead.

Original comment by steven.b...@gmail.com on 28 Apr 2011 at 9:14