Ilya, Anton, Ivan, hi! I fix some comments you leave in the PR. Also I checked some test suites and found that some tests are written in the core module but depend on the indexing module (or other modules). Some of such test classes contain tests that are related to the core functionality, but some to indexing. I'm not sure if it is correct to move a whole suite with all tests from the indexing module to the core, as it will hide some core tests from the core module.
I believe that the correct solution is to investigate every such test and move it to the right module. But I think this work will take a lot of time and should be performed in a separate ticket, I will do it in the background. I think currently we should proceed with a way I introduced in PR: 1. Create fake suites for all such tests (written in core, suited in other modules: indexing/spring/zookeeper/etc) in the core module. I named such suites with prefix Core*. 2. Replace tests in modules with links to fake suites. 3. Create an umbrella Jira ticket to discover every fake suite and replace it with a new one in the right module. 4. Merge this PR for introducing a new travis check to avoid losing new tests. WDYT? List of such mixed suites: 1. suite IgniteBinaryCacheQueryTestSuite test GridCacheQueryIndexingDisabledSelfTest test IgniteCacheBinaryObjectsScanSelfTest test IgniteCacheBinaryObjectsScanWithEventsSelfTest) 2. suite IgniteCacheQuerySelfTestSuite3 test GridCacheContinuousQueryNodesFilteringTest 3. suite IgniteCacheQuerySelfTestSuite5 test ContinuousQueryRemoteFilterMissingInClassPathSelfTest 4. suite IgniteCacheQuerySelfTestSuite6 test CacheContinuousQueryOperationP2PTest test CacheContinuousQueryFilterDeploymentFailedTest 5. all tests in suite IgnitePdsWithIndexingCoreTestSuite 6. and some others. On Wed, Nov 18, 2020 at 12:38 PM Max Timonin <timonin.ma...@gmail.com> wrote: > Hi Ilya! Thank you for up the topic. I will come back with fixes and > comments in a couple of days. > > On Tue, Nov 17, 2020 at 4:26 PM Ilya Kasnacheev <ilya.kasnach...@gmail.com> > wrote: > >> Hello! >> >> I have left some comments and there's also more discussion there. Can you >> please look? >> >> Thanks, >> -- >> Ilya Kasnacheev >> >> >> вт, 3 нояб. 2020 г. в 00:03, Max Timonin <timonin.ma...@gmail.com>: >> >> > Hi! >> > >> > I've updated PR: https://github.com/apache/ignite/pull/8367. Anton, >> Ivan, >> > Ivan could you please review it? >> > >> > Some moments to mention: >> > 1. I've added new suites: SerializerSuite >> (ignite-cassandra-serializers), >> > DistanceTestSuite, NaiveBayesTestSuite (ignite-ml). Should we configure >> a >> > TeamCity to run them? >> > >> > 2. Some tests marked as failed, I'll create corresponding tickets for >> them >> > after PR approved: >> > - IgnitePKIndexesMigrationToUnwrapPkTest >> > - P2PGridifySelfTest >> > - GridCacheMultithreadedFailoverAbstractTest >> > - WalCompactionAfterRestartTest >> > - GridTcpCommunicationSpiLogTest >> > - ComplexSecondaryKeyUnwrapSelfTest >> > - SqlTransactionsSelfTest >> > >> > 3. Add docs to DEVNOTES.txt >> > >> > On Mon, Nov 2, 2020 at 11:44 AM Anton Vinogradov <a...@apache.org> wrote: >> > >> > > > As I understand we >> > > > can't just move suites between modules, as TeamCity may depend on >> the >> > > path >> > > > to them. >> > > See no problem to update TC as well. >> > > >> > > On Fri, Oct 30, 2020 at 4:32 PM Ivan Daschinsky <ivanda...@gmail.com> >> > > wrote: >> > > >> > > > I suggests to mark these tests with @Ignore and file tickets to fix >> > them. >> > > > >> > > > пт, 30 окт. 2020 г. в 16:26, Ivan Daschinsky <ivanda...@gmail.com>: >> > > > >> > > > > Hi >> > > > > >> > > > > WalCompactionAfterRestartTest -- yes we need it. This test failed >> > > because >> > > > > of race (test shold be rewritten a little bit) >> > > > > >> > > > > пт, 30 окт. 2020 г. в 16:15, Max Timonin <timonin.ma...@gmail.com >> >: >> > > > > >> > > > >> Hi! >> > > > >> >> > > > >> Yes, you're correct. I've developed the check and started to >> clean >> > > tests >> > > > >> (move them to suites, mark some tests with Ignore, etc.). I >> finish >> > > work >> > > > on >> > > > >> the core module. I hope it was the biggest one, and others are >> less. >> > > If >> > > > >> so, >> > > > >> I think I will finish the work on other modules in 1 or 2 weeks, >> as >> > I >> > > do >> > > > >> this activity in the background (~10% of my work time). Actually >> > I've >> > > > >> found >> > > > >> 3 failed tests in the core module that aren't in any suite, so I >> > need >> > > > time >> > > > >> to discover reason of failures and if we actually need those >> tests: >> > > > >> >> > > > >> GridCacheMultithreadedFailoverAbstractTest >> > > > >> WalCompactionAfterRestartTest >> > > > >> GridTcpCommunicationSpiLogTest >> > > > >> >> > > > >> Also we should decide how to be with wrongly located es. As I >> > > understand >> > > > >> we >> > > > >> can't just move suites between modules, as TeamCity may depend on >> > the >> > > > path >> > > > >> to them. So, for such cases I've just created suites in the right >> > > > module, >> > > > >> and replaced the test list with the new class suite. It does not >> > look >> > > > >> pretty enough, but I think It's a path of least resistance. WDYT? >> > > > >> >> > > > >> BEFORE: >> > > > >> Module A -> SuiteA -> testA1, testA2, testB1, testB2 >> > > > >> Module B -> testB1, testB2 >> > > > >> >> > > > >> AFTER: >> > > > >> Module A -> SuiteA, SuiteB >> > > > >> Module B -> SuiteB -> testB1, testB2 >> > > > >> >> > > > >> On Fri, Oct 30, 2020 at 3:38 PM Anton Vinogradov <a...@apache.org> >> > > wrote: >> > > > >> >> > > > >> > Folks, >> > > > >> > What's the current state of this thread? >> > > > >> > AFAIU, we found unused and wrongly located tests and developed >> > some >> > > > >> > checker, could we split this to some PRs? >> > > > >> > Let's merge tests usage fix and location fixes first, this will >> > > > provide >> > > > >> us >> > > > >> > an ability to automate check using Travis. >> > > > >> > >> > > > >> > On Tue, Oct 20, 2020 at 12:06 PM Ivan Pavlukhin < >> > > vololo...@gmail.com> >> > > > >> > wrote: >> > > > >> > >> > > > >> > > Max, Ivan, >> > > > >> > > >> > > > >> > > Using explicit @Ignore and the automated check sounds good to >> > me. >> > > If >> > > > >> > > nobody has arguments against it I think we should do it. >> > > > >> > > >> > > > >> > > 2020-10-19 19:30 GMT+03:00, Max Timonin < >> > timonin.ma...@gmail.com >> > > >: >> > > > >> > > > Hi Ivan, >> > > > >> > > > >> > > > >> > > > I've checked the ticket you provide. It contains subtasks >> to >> > > > >> uncomment >> > > > >> > or >> > > > >> > > > to remove some unused tests. It definitely describes some >> > cases >> > > > I've >> > > > >> > > found. >> > > > >> > > > So what do you think if I uncomment them in suites, add >> > @Ignore >> > > > >> > > annotation >> > > > >> > > > for those tests while the tickets are open? This will help >> to >> > > find >> > > > >> out >> > > > >> > > > tests that were forgiven in a recent time. >> > > > >> > > > >> > > > >> > > > Also I believe that this check must be automated. I didn't >> > find >> > > a >> > > > >> way >> > > > >> > how >> > > > >> > > > uncomment / unused tests are found in the ticket. If there >> is >> > no >> > > > >> any - >> > > > >> > I >> > > > >> > > > propose mine PR for this purpose. >> > > > >> > > > >> > > > >> > > > >> > > > >> > > > >> > > > >> > > > On Mon, Oct 19, 2020 at 5:24 PM Ivan Daschinsky < >> > > > >> ivanda...@gmail.com> >> > > > >> > > > wrote: >> > > > >> > > > >> > > > >> > > >> Ivan, as far as I understand, Max also created >> verification >> > > check >> > > > >> for >> > > > >> > > not >> > > > >> > > >> included test and found a few tests, that have never been >> > > > included >> > > > >> in >> > > > >> > > any >> > > > >> > > >> testsuites. >> > > > >> > > >> >> > > > >> > > >> Also, I suppose, that even if we cannot run some tests, >> these >> > > > tests >> > > > >> > > >> should >> > > > >> > > >> be ignored using annotation, but not commented. >> > > > >> > > >> >> > > > >> > > >> пн, 19 окт. 2020 г. в 16:33, Ivan Pavlukhin < >> > > vololo...@gmail.com >> > > > >: >> > > > >> > > >> >> > > > >> > > >> > Hi Max, >> > > > >> > > >> > >> > > > >> > > >> > There is an existing effort about "abandoned" tests >> > > > >> > > >> > https://issues.apache.org/jira/browse/IGNITE-9210 >> > > > >> > > >> > >> > > > >> > > >> > 2020-10-19 16:25 GMT+03:00, Max Timonin < >> > > > timonin.ma...@gmail.com >> > > > >> >: >> > > > >> > > >> > > Hi Igniters! >> > > > >> > > >> > > >> > > > >> > > >> > > I made a research into tests that aren't included in >> any >> > > test >> > > > >> > suite. >> > > > >> > > >> > > As >> > > > >> > > >> > > TeamCity runs tests by suites so there could be tests >> > that >> > > > >> never >> > > > >> > run >> > > > >> > > >> > > on >> > > > >> > > >> > TC. >> > > > >> > > >> > > So I tried implementing a simple check for such tests >> and >> > > > >> include >> > > > >> > it >> > > > >> > > >> > > in >> > > > >> > > >> > > Ignite's travis config. >> > > > >> > > >> > > >> > > > >> > > >> > > The check runs while "mvn test" command and >> piggy-backs >> > on >> > > > the >> > > > >> > maven >> > > > >> > > >> > > surefire plugin. I replaced the junit provider with a >> > > custom >> > > > >> one >> > > > >> > > that >> > > > >> > > >> > > checks if a class is a test or a suite (there are some >> > > Ignite >> > > > >> > > >> > > specific >> > > > >> > > >> > > stuff), marks tests that are in suites and raises an >> > > > exception >> > > > >> if >> > > > >> > > >> > > there >> > > > >> > > >> > are >> > > > >> > > >> > > non-suited tests. It's implemented as a part of maven >> > > command >> > > > >> so >> > > > >> > it >> > > > >> > > >> runs >> > > > >> > > >> > > for every module separately. >> > > > >> > > >> > > >> > > > >> > > >> > > I've prepared draft PR with this check: >> > > > >> > > >> > > https://github.com/apache/ignite/pull/8367 >> > > > >> > > >> > > Travis check report is here: >> > > > >> > > >> > > >> > https://travis-ci.org/github/apache/ignite/jobs/737046387 >> > > > >> > > >> > > As It's a draft, so I skip some maven configuration >> steps >> > > > for a >> > > > >> > > >> > > while. >> > > > >> > > >> > Also >> > > > >> > > >> > > I run the check only for the core module. >> > > > >> > > >> > > >> > > > >> > > >> > > But I have some results that want to discuss before >> > > continue >> > > > >> the >> > > > >> > > >> > > work: >> > > > >> > > >> > > 1. Currently in the core module there are 53 tests >> that >> > > > aren't >> > > > >> > part >> > > > >> > > >> > > of >> > > > >> > > >> > any >> > > > >> > > >> > > test suite. I'm not sure about the reason for every >> test. >> > > So >> > > > I >> > > > >> > just >> > > > >> > > >> > > put >> > > > >> > > >> > > below a list of the tests and last contributor to a >> file >> > > that >> > > > >> > > >> > > contains >> > > > >> > > >> a >> > > > >> > > >> > > test. >> > > > >> > > >> > > >> > > > >> > > >> > > 2. Some tests are located in the core module, but >> suites >> > > are >> > > > >> in a >> > > > >> > > >> > > different, for example ignite-indexing suite >> > > > >> > > >> > > IgniteCacheQuerySelfTestSuite3 contains >> > > > >> > > >> > > only tests written in the core module, and none from >> the >> > > > >> indexing >> > > > >> > > >> module. >> > > > >> > > >> > > Also there are suites in spring, uri-deploy, zookeeper >> > > > >> modules. In >> > > > >> > > my >> > > > >> > > >> PR >> > > > >> > > >> > > I've just copied the test suites to the core module. >> > > > >> > > >> > > >> > > > >> > > >> > > 3. Some test classes are named with the "Abstract" >> suffix >> > > but >> > > > >> > don't >> > > > >> > > >> have >> > > > >> > > >> > > the corresponding modifier (for example, >> > > > >> > > >> > > IgniteTxTimeoutAbstractTest). >> > > > >> > > >> > So, >> > > > >> > > >> > > I add the modifier for every such file if it's not a >> part >> > > of >> > > > >> any >> > > > >> > > >> > > suite. >> > > > >> > > >> > > >> > > > >> > > >> > > What do you think about this check? If Ignite needs >> it, >> > > let's >> > > > >> > > discuss >> > > > >> > > >> > next >> > > > >> > > >> > > things: >> > > > >> > > >> > > 1. Mark tests that should never be in any suite by >> some >> > > > reason; >> > > > >> > > >> > > 2. Fix the missed tests; >> > > > >> > > >> > > 3. How to declare suites that contains tests from a >> > > different >> > > > >> > > module; >> > > > >> > > >> > > 4. How to check if TC runs all suites. >> > > > >> > > >> > > >> > > > >> > > >> > > List of non-suited tests in the core module: >> > > > >> > > >> > > >> > > > >> > > >> > > maksim.stepac...@gmail.com: >> > > > >> > > >> > > GridTcpCommunicationSpiLogTest >> > > > >> > > >> > > >> > > > >> > > >> > > nizhi...@apache.org: >> > > > >> > > >> > > >> IgniteCacheClientMultiNodeUpdateTopologyLockTest >> > > > >> > > >> > > CacheClientsConcurrentStartTest >> > > > >> > > >> > > IgniteOutOfMemoryPropagationTest >> > > > >> > > >> > > GridCacheP2PUndeploySelfTest >> > > > >> > > >> > > GridCacheRebalancingOrderingTest >> > > > >> > > >> > > IgniteMassLoadSandboxTest >> > > > >> > > >> > > PageLockTrackerMXBeanImplTest >> > > > >> > > >> > > IgniteBinaryMetadataUpdateNodeRestartTest >> > > > >> > > >> > > CacheLockCandidatesThreadTest >> > > > >> > > >> > > GridMBeanBaselineTest >> > > > >> > > >> > > RendezvousAffinityFunctionSimpleBenchmark >> > > > >> > > >> > > >> > > > >> > > >> > > samvi...@yandex.ru: >> > > > >> > > >> > > IgnitePdsNoSpaceLeftOnDeviceTest >> > > > >> > > >> > > >> > > > >> > > >> > > maxmu...@gmail.com: >> > > > >> > > >> > > GridCacheOnCopyFlagReplicatedSelfTest >> > > > >> > > >> > > GridCacheOnCopyFlagLocalSelfTest >> > > > >> > > >> > > >> GridCacheReplicatedAtomicReferenceMultiNodeTest >> > > > >> > > >> > > GridCacheReplicatedMarshallerTxTest >> > > > >> > > >> > > GridCacheReplicatedTxConcurrentGetTest >> > > > >> > > >> > > GridCacheOnCopyFlagTxPartitionedSelfTest >> > > > >> > > >> > > GridCacheReplicatedTxReadTest >> > > > >> > > >> > > >> GridCachePartitionedAtomicReferenceMultiNodeTest >> > > > >> > > >> > > GridCacheOnCopyFlagAtomicSelfTest >> > > > >> > > >> > > >> > > > >> > > >> > > mmu...@apache.org: >> > > > >> > > >> > > GridActivateExtensionTest >> > > > >> > > >> > > IgniteChangeGlobalStateCacheTest >> > > > >> > > >> > > IgniteChangeGlobalStateTest >> > > > >> > > >> > > IgniteChangeGlobalStateServiceTest >> > > > >> > > >> > > IgniteChangeGlobalStateDataStructureTest >> > > > >> > > >> > > >> > > > >> > > >> > > oignate...@gridgain.com: >> > > > >> > > >> > > CacheEntryProcessorCopySelfTest >> > > > >> > > >> > > MemoryLeaksOnRestartNodeTest >> > > > >> > > >> > > GridCacheAtomicPreloadSelfTest >> > > > >> > > >> > > WalCompactionAfterRestartTest >> > > > >> > > >> > > IgniteCacheConcurrentPutGetRemove >> > > > >> > > >> > > GridIoManagerBenchmark0 >> > > > >> > > >> > > >> > > > >> > > >> > > nsamelc...@gmail.com: >> > > > >> > > >> > > GridLongRunningInitNewCrdFutureDiagnosticsTest >> > > > >> > > >> > > GridCacheMultithreadedFailoverAbstractTest >> > > > >> > > >> > > >> > > > >> > > >> > > alexey.goncha...@gmail.com: >> > > > >> > > >> > > GridCacheBinaryObjectsAtomicOnheapSelfTest >> > > > >> > > >> > > >> > > > GridCacheBinaryObjectsAtomicNearDisabledOnheapSelfTest >> > > > >> > > >> > > >> GridCacheBinaryObjectsPartitionedOnheapSelfTest >> > > > >> > > >> > > >> > > > >> > GridCacheBinaryObjectsPartitionedNearDisabledOnheapSelfTest >> > > > >> > > >> > > >> > > > >> > > >> > > vladis...@gmail.com: >> > > > >> > > >> > > IgnitePartitionedLockSelfTest >> > > > >> > > >> > > >> > > > >> > > >> > > alexandr.bel...@xored.com: >> > > > >> > > >> > > IgniteStableBaselineCachePutAllFailoverTest >> > > > >> > > >> > > IgniteStableBaselineCacheRemoveFailoverTest >> > > > >> > > >> > > >> > > > >> > > >> > > ilant...@gridgain.com: >> > > > >> > > >> > > IgniteCacheAtomicOnheapExpiryPolicyTest >> > > > >> > > >> > > IgniteCacheAtomicLocalOnheapExpiryPolicyTest >> > > > >> > > >> > > GridCacheReplicatedOnheapFullApiSelfTest >> > > > >> > > >> > > GridCacheBinaryObjectsLocalOnheapSelfTest >> > > > >> > > >> > > >> > > > >> > > >> > > oignate...@users.noreply.github.com: >> > > > >> > > >> > > GridCacheTtlManagerEvictionSelfTest >> > > > >> > > >> > > >> > > > >> > > >> > > ira...@apache.org: >> > > > >> > > >> > > CommonPoolStarvationCheckpointTest >> > > > >> > > >> > > >> > > > >> > > >> > > alievmi...@gmail.com: >> > > > >> > > >> > > RemoveAllDeadlockTest >> > > > >> > > >> > > >> > > > >> > > >> > > schugu...@gridgain.com: >> > > > >> > > >> > > FullyConnectedComponentSearcherTest >> > > > >> > > >> > > >> > > > >> > > >> > > sboi...@gridgain.com: >> > > > >> > > >> > > IgniteDataStructuresNoClassOnServerTest >> > > > >> > > >> > > >> > > > >> > > >> > > timonin.ma...@gmail.com: >> > > > >> > > >> > > ReliableChannelTest >> > > > >> > > >> > > ThinClientPartitionAwarenessDiscoveryTest >> > > > >> > > >> > > >> > > > >> > > >> > >> > > > >> > > >> > >> > > > >> > > >> > -- >> > > > >> > > >> > >> > > > >> > > >> > Best regards, >> > > > >> > > >> > Ivan Pavlukhin >> > > > >> > > >> > >> > > > >> > > >> >> > > > >> > > >> >> > > > >> > > >> -- >> > > > >> > > >> Sincerely yours, Ivan Daschinskiy >> > > > >> > > >> >> > > > >> > > > >> > > > >> > > >> > > > >> > > >> > > > >> > > -- >> > > > >> > > >> > > > >> > > Best regards, >> > > > >> > > Ivan Pavlukhin >> > > > >> > > >> > > > >> > >> > > > >> >> > > > > >> > > > > >> > > > > -- >> > > > > Sincerely yours, Ivan Daschinskiy >> > > > > >> > > > >> > > > >> > > > -- >> > > > Sincerely yours, Ivan Daschinskiy >> > > > >> > > >> > >> >
