>> WDYT? LGTM On Wed, Nov 25, 2020 at 12:23 AM Max Timonin <timonin.ma...@gmail.com> wrote:
> 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 > >> > > > > >> > > > >> > > >> > > >
