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 > > > > > >
