+1 to Kirk's comments. Also, regarding (c), using AssumeThat [1] (or, alternatively & IMO preferrably, [2]) might provide some temporary relief.
[1] https://junit.org/junit4/javadoc/latest/org/junit/Assume.html [2] https://joel-costigliola.github.io/assertj/core-8/api/org/assertj/core/api/Assumptions.html On Thu, Jan 2, 2020 at 11:46 AM Kirk Lund <kl...@apache.org> wrote: > The Geode code base has 328 tests across 145 files with @Ignore. The vast > majority of these were disabled pre-Geode because the previous group's > policy was to disable any test that failed in CI, then file a bug system > ticket, fix it, and re-enable it. However, the process never followed > through beyond disabling the test and filing a ticket. Since that was the > old pre-Apache bug system, most of us don't have access to it. > > From a Pivotal point-of-view, it's possible that every Geode commit that > passes precheckin but then breaks a Pivotal Hydra test could have been > caught by one of those disabled tests. So we are probably already > experiencing pain and paying the price for not re-enabling these tests (in > at least some cases). > > @Ignore was introduced in JUnit 4, but most of these tests were JUnit 3 > style. After I upgraded us to JUnit 4, I found every JUnit 3 style of test > that had been disabled by renaming the test, fixed the name of the test, > and added @Test plus @Ignore so that we can at least be aware of how many > tests are disabled and actively search for them. I tried to add include a > comment in the annotation if I could figure out why it was disabled. Some > of these comments are things like (a) TRAC ticket # if known, (b) not yet > implemented, (c) skip test for this configuration, (d) not yet implemented > for partitioned region, etc. Sometimes I just reused a java comment that > the disabler left above the disabled test. > > More info on some of those @Ignore comments: > > (b) not yet implemented -- this means probably means the test was never > finished and should just be deleted but it might also be another example of > (c) or (d) in certain test classes > > (c) skip test for this configuration -- this is a class that extends > another test class but one or more test methods are overridden with @Ignore > to prevent the test from running in the subclass because it's invalid for > that configuration -- we can't just delete these, we have to remove > class-hierarchy (stop extending test classes!) which will result in some > redundancy between two test classes > > I do not think we should blindly delete them all, and I think we need to > spend time studying each one individually to determine what the state of > the test is and what should be done for it. So even if many in our > community want to just delete all tests with @Ignore, that's not going to > work in at least half of the test classes. You'll have to tease apart > sub-classes from super-classes. > > Plus you won't actually know that the test has other coverage unless you > spend the time analyzing it, which is what I think we should do. > > Some quick examples: > > *1) ClientServerTimeSyncDUnitTest* > > Notes: this test class has only two tests and both are @Ignored, so maybe > deleting them isn't the best answer. > > testClientTimeAdvances is marked with @Ignore("Bug 52327"). #52327 is > "ClientServerTimeSyncDUnitTest.testClientTimeAdvances fails intermittently" > > testClientTimeSlowsDown is marked with @Ignore("not yet implemented") which > either means the test was never finished or the functionality it's trying > to test was never finished in the product code. It looks like the test > never had any assertions. > > Personally, I don't think I'd want to delete either of these tests. I would > prefer to fix them up. > > If we really care about Apache Geode, our test coverage, and the quality of > the product, then I think we should separate super-sub classes that force > us to disable test in sub-class that are invalid test cases and fix analyze > each test on a case-by-case basis to determine what should be done. And I > believe that the default approach for each test should be to fix it rather > than delete it. > > *2) DistributedAckPersistentRegionCCEDUnitTest* > > > geode-core/src/distributedTest/java/org/apache/geode/cache30/DistributedAckPersistentRegionCCEDUnitTest.java:41: > @Ignore("Skip test for this configuration") > > geode-core/src/distributedTest/java/org/apache/geode/cache30/DistributedAckPersistentRegionCCEDUnitTest.java:46: > @Ignore("Skip test for this configuration") > > geode-core/src/distributedTest/java/org/apache/geode/cache30/DistributedAckPersistentRegionCCEDUnitTest.java:51: > @Ignore("Skip test for this configuration") > > geode-core/src/distributedTest/java/org/apache/geode/cache30/DistributedAckRegionCCEDUnitTest.java:110: > @Ignore("replicates don't allow local destroy") > > geode-core/src/distributedTest/java/org/apache/geode/cache30/DistributedAckRegionCCEDUnitTest.java:117: > @Ignore("replicates don't allow local destroy") > > geode-core/src/distributedTest/java/org/apache/geode/cache30/DistributedAckRegionCCEDUnitTest.java:294: > @Ignore("Enable after fix for bug GEODE-1891 was #45704") > > Here you see 3 test methods with "Skip test for this configuration". If you > delete these from the sub-class, it'll run these three tests and probably > blow up because it's an invalid configuration. To fix this we'll have to > NOT extend the super-classes and start inlining super-classes and deleting > what's not needed. > > Next are 2 test methods with "replicates don't allow local destroy" -- > sounds like the same thing as "Skip test for this configuration" to me. > > Last one is "Enable after fix for bug GEODE-1891 was #45704". This > represents product functionality that doesn't work how we want it to AND we > have a test that demonstrates it. Seems a shame to delete this test. If a > developer ever tries to fix GEODE-1891, they'll want this test. I suppose > you could copy-paste the test into the comments or attachments of > GEODE-1891 and delete it from code. > > *3) GlobalRegionCCEDUnitTest* > > > geode-core/src/distributedTest/java/org/apache/geode/cache30/GlobalRegionCCEDUnitTest.java:101: > @Ignore("replicates don't allow local destroy") > > geode-core/src/distributedTest/java/org/apache/geode/cache30/GlobalRegionCCEDUnitTest.java:108: > @Ignore("replicates don't allow local destroy") > > geode-core/src/distributedTest/java/org/apache/geode/cache30/GlobalRegionCCEDUnitTest.java:137: > @Ignore("TODO: takes too long with global regions and cause false dunit > hangs") > > geode-core/src/distributedTest/java/org/apache/geode/cache30/GlobalRegionCCEDUnitTest.java:143: > @Ignore("TODO: takes too long with global regions and cause false dunit > hangs") > > geode-core/src/distributedTest/java/org/apache/geode/cache30/GlobalRegionCCEDUnitTest.java:149: > @Ignore("TODO: takes too long with global regions and cause false dunit > hangs") > > geode-core/src/distributedTest/java/org/apache/geode/cache30/GlobalRegionCCEDUnitTest.java:221: > @Ignore("Disabling due to bug #52347") > > geode-core/src/distributedTest/java/org/apache/geode/cache30/GlobalRegionCCEDUnitTest.java:231: > @Ignore("TODO: reenable this test") > > Again we have sub-class overriding and ignoring tests that are invalid with > "replicates don't allow local destroy". Next we have "TODO: takes too long > with global regions". Apparently someone a long time ago decided these 3 > tests take too long and disabled them. Sure seems like we should re-enable > them to me! If they take too long, then look for sleeps. If GLOBAL takes > too long but any users still use it too bad. If GLOBAL takes too long but > NO users still use, then we deprecate GLOBAL, release Geode 2.0 and delete > it! > > #52347 is "testConcurrentEventsOnEmptyRegion fails intermittently in CI". > We could look into who filed #52347 and assign fixing the flakiness in this > test method to that developer. > > "TODO: reenable this test" -- I guess we should reenable this test and see > how flaky it is?! After all that's what it says to do. > > I quick glance over all of the results of $ git grep "@Ignore" shows that > the above is pretty much the sort of things we're looking at. > > I'm 100% against deleting them. If everyone else wants to delete them, then > I propose reassigning me to do nothing but fix ALL @Ignored tests and I'll > jump on it. > > And just to be very clear: > > -1 to all proposals to delete all tests with @Ignore (I can't think of any > argument to change me to -0) > > -Kirk > > On Tue, Dec 31, 2019 at 2:08 PM Alexander Murmann <amurm...@apache.org> > wrote: > > > Here are a few things that are true for me or I believe are true in > > general: > > > > - Our test suite is more flaky than we'd like it to be > > - I don't believe that adding more Unit tests that follow existing > > patterns buys us that much. I'd rather see something similar to what > > some > > folks are doing with Membership right now where we isolate the code > and > > test it more systematically > > - We have other testing gaps: We have benchmarks 👏🎉, but we are > still > > lacking coverage in that ares; our community is still lacking HA > tests. > > I'd > > rather fill those than bring back old DUnit tests that are chosen > > somewhat > > at random. > > - I'd rather be deliberate about what tests we introduce than > wholesale > > bring back a set of tests, since any of these re-introduced tests has > a > > potential to be flaky. Let's make sure our tests carry their weight. > > - If we delete these tests, we can always go back to a SHA from today > > and bring them back at a later date > > - These tests have been ignored since a very long time and we've > shipped > > without them and nobody has missed them enough to bring them back. > > > > Given all the above, my vote is for less noise in our code, which means > > deleting all ignored tests. If we want to keep them, I'd love to hear a > > plan of action on how we bring them back. Having a bunch of dead code > helps > > nobody. > > > > On Tue, Dec 31, 2019 at 1:50 PM Mark Hanson <mhan...@pivotal.io> wrote: > > > > > Hi All, > > > > > > As part of what I am doing to fix flaky tests, I periodically come > across > > > tests that are @Ignore’d. I am curious what we would like to do with > them > > > generally speaking. We could fix them, which would seem obvious, but we > > are > > > struggling to fix flaky tests as it is. We could delete them, but > those > > > tests were written for a reason (I hope). Or we could leave them. This > > > pollutes searches etc as inactive code requiring upkeep at least. > > > > > > I don’t have an easy answer. Some have suggested deleting them. I tend > to > > > lean that direction, but I thought I would consult the community for a > > > broader perspective. > > > > > > Thanks, > > > Mark > > > -- -John Spring Data Team
