ignite-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Anton Vinogradov ...@apache.org>
Subject Re: [DISCUSS] Missed (non-suited) tests
Date Wed, 25 Nov 2020 11:49:10 GMT
>> WDYT?
LGTM

On Wed, Nov 25, 2020 at 12:23 AM Max Timonin <timonin.maxim@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.maxim@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.kasnacheev@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.maxim@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 <av@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 <
> ivandasch@gmail.com>
> >> > > wrote:
> >> > >
> >> > > > I suggests to mark these tests with @Ignore and file tickets
to
> fix
> >> > them.
> >> > > >
> >> > > > пт, 30 окт. 2020 г. в 16:26, Ivan Daschinsky <ivandasch@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.maxim@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 <
> av@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
<
> >> > > vololo100@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.maxim@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 <
> >> > > > >> ivandasch@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 <
> >> > > vololo100@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.maxim@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.stepachev@gmail.com:
> >> > > > >> > > >> > >         GridTcpCommunicationSpiLogTest
> >> > > > >> > > >> > >
> >> > > > >> > > >> > > nizhikov@apache.org:
> >> > > > >> > > >> > >
> >>  IgniteCacheClientMultiNodeUpdateTopologyLockTest
> >> > > > >> > > >> > >         CacheClientsConcurrentStartTest
> >> > > > >> > > >> > >         IgniteOutOfMemoryPropagationTest
> >> > > > >> > > >> > >         GridCacheP2PUndeploySelfTest
> >> > > > >> > > >> > >         GridCacheRebalancingOrderingTest
> >> > > > >> > > >> > >         IgniteMassLoadSandboxTest
> >> > > > >> > > >> > >         PageLockTrackerMXBeanImplTest
> >> > > > >> > > >> > >         IgniteBinaryMetadataUpdateNodeRestartTest
> >> > > > >> > > >> > >         CacheLockCandidatesThreadTest
> >> > > > >> > > >> > >         GridMBeanBaselineTest
> >> > > > >> > > >> > >         RendezvousAffinityFunctionSimpleBenchmark
> >> > > > >> > > >> > >
> >> > > > >> > > >> > > samvimes@yandex.ru:
> >> > > > >> > > >> > >         IgnitePdsNoSpaceLeftOnDeviceTest
> >> > > > >> > > >> > >
> >> > > > >> > > >> > > maxmuzaf@gmail.com:
> >> > > > >> > > >> > >         GridCacheOnCopyFlagReplicatedSelfTest
> >> > > > >> > > >> > >         GridCacheOnCopyFlagLocalSelfTest
> >> > > > >> > > >> > >
> >>  GridCacheReplicatedAtomicReferenceMultiNodeTest
> >> > > > >> > > >> > >         GridCacheReplicatedMarshallerTxTest
> >> > > > >> > > >> > >         GridCacheReplicatedTxConcurrentGetTest
> >> > > > >> > > >> > >         GridCacheOnCopyFlagTxPartitionedSelfTest
> >> > > > >> > > >> > >         GridCacheReplicatedTxReadTest
> >> > > > >> > > >> > >
> >>  GridCachePartitionedAtomicReferenceMultiNodeTest
> >> > > > >> > > >> > >         GridCacheOnCopyFlagAtomicSelfTest
> >> > > > >> > > >> > >
> >> > > > >> > > >> > > mmuzaf@apache.org:
> >> > > > >> > > >> > >         GridActivateExtensionTest
> >> > > > >> > > >> > >         IgniteChangeGlobalStateCacheTest
> >> > > > >> > > >> > >         IgniteChangeGlobalStateTest
> >> > > > >> > > >> > >         IgniteChangeGlobalStateServiceTest
> >> > > > >> > > >> > >         IgniteChangeGlobalStateDataStructureTest
> >> > > > >> > > >> > >
> >> > > > >> > > >> > > oignatenko@gridgain.com:
> >> > > > >> > > >> > >         CacheEntryProcessorCopySelfTest
> >> > > > >> > > >> > >         MemoryLeaksOnRestartNodeTest
> >> > > > >> > > >> > >         GridCacheAtomicPreloadSelfTest
> >> > > > >> > > >> > >         WalCompactionAfterRestartTest
> >> > > > >> > > >> > >         IgniteCacheConcurrentPutGetRemove
> >> > > > >> > > >> > >         GridIoManagerBenchmark0
> >> > > > >> > > >> > >
> >> > > > >> > > >> > > nsamelchev@gmail.com:
> >> > > > >> > > >> > >
>  GridLongRunningInitNewCrdFutureDiagnosticsTest
> >> > > > >> > > >> > >         GridCacheMultithreadedFailoverAbstractTest
> >> > > > >> > > >> > >
> >> > > > >> > > >> > > alexey.goncharuk@gmail.com:
> >> > > > >> > > >> > >         GridCacheBinaryObjectsAtomicOnheapSelfTest
> >> > > > >> > > >> > >
> >> > > >  GridCacheBinaryObjectsAtomicNearDisabledOnheapSelfTest
> >> > > > >> > > >> > >
> >>  GridCacheBinaryObjectsPartitionedOnheapSelfTest
> >> > > > >> > > >> > >
> >> > > > >> >  GridCacheBinaryObjectsPartitionedNearDisabledOnheapSelfTest
> >> > > > >> > > >> > >
> >> > > > >> > > >> > > vladisavj@gmail.com:
> >> > > > >> > > >> > >         IgnitePartitionedLockSelfTest
> >> > > > >> > > >> > >
> >> > > > >> > > >> > > alexandr.belyak@xored.com:
> >> > > > >> > > >> > >         IgniteStableBaselineCachePutAllFailoverTest
> >> > > > >> > > >> > >         IgniteStableBaselineCacheRemoveFailoverTest
> >> > > > >> > > >> > >
> >> > > > >> > > >> > > ilantukh@gridgain.com:
> >> > > > >> > > >> > >         IgniteCacheAtomicOnheapExpiryPolicyTest
> >> > > > >> > > >> > >         IgniteCacheAtomicLocalOnheapExpiryPolicyTest
> >> > > > >> > > >> > >         GridCacheReplicatedOnheapFullApiSelfTest
> >> > > > >> > > >> > >         GridCacheBinaryObjectsLocalOnheapSelfTest
> >> > > > >> > > >> > >
> >> > > > >> > > >> > > oignatenko@users.noreply.github.com:
> >> > > > >> > > >> > >         GridCacheTtlManagerEvictionSelfTest
> >> > > > >> > > >> > >
> >> > > > >> > > >> > > irakov@apache.org:
> >> > > > >> > > >> > >         CommonPoolStarvationCheckpointTest
> >> > > > >> > > >> > >
> >> > > > >> > > >> > > alievmirza@gmail.com:
> >> > > > >> > > >> > >         RemoveAllDeadlockTest
> >> > > > >> > > >> > >
> >> > > > >> > > >> > > schugunov@gridgain.com:
> >> > > > >> > > >> > >         FullyConnectedComponentSearcherTest
> >> > > > >> > > >> > >
> >> > > > >> > > >> > > sboikov@gridgain.com:
> >> > > > >> > > >> > >         IgniteDataStructuresNoClassOnServerTest
> >> > > > >> > > >> > >
> >> > > > >> > > >> > > timonin.maxim@gmail.com:
> >> > > > >> > > >> > >         ReliableChannelTest
> >> > > > >> > > >> > >         ThinClientPartitionAwarenessDiscoveryTest
> >> > > > >> > > >> > >
> >> > > > >> > > >> >
> >> > > > >> > > >> >
> >> > > > >> > > >> > --
> >> > > > >> > > >> >
> >> > > > >> > > >> > Best regards,
> >> > > > >> > > >> > Ivan Pavlukhin
> >> > > > >> > > >> >
> >> > > > >> > > >>
> >> > > > >> > > >>
> >> > > > >> > > >> --
> >> > > > >> > > >> Sincerely yours, Ivan Daschinskiy
> >> > > > >> > > >>
> >> > > > >> > > >
> >> > > > >> > >
> >> > > > >> > >
> >> > > > >> > > --
> >> > > > >> > >
> >> > > > >> > > Best regards,
> >> > > > >> > > Ivan Pavlukhin
> >> > > > >> > >
> >> > > > >> >
> >> > > > >>
> >> > > > >
> >> > > > >
> >> > > > > --
> >> > > > > Sincerely yours, Ivan Daschinskiy
> >> > > > >
> >> > > >
> >> > > >
> >> > > > --
> >> > > > Sincerely yours, Ivan Daschinskiy
> >> > > >
> >> > >
> >> >
> >>
> >
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message