ignite-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Max Timonin <timonin.ma...@gmail.com>
Subject Re: [DISCUSS] Missed (non-suited) tests
Date Tue, 24 Nov 2020 21:21:55 GMT
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