ignite-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Dmitriy Pavlov <dpav...@apache.org>
Subject Re: Is it time to move forward to JUnit4 (5)?
Date Wed, 19 Dec 2018 10:27:42 GMT
Hi Igniters,

I've tried yesterday to find any of @Ignore d tests with a link to the
issue in its value on TeamCity, but I did not manage to.

Do you know if JUnit Ignored tests are completely invisible in the TC and
its REST? I've also tried to google it, but not found anything yet.

I remember that .NET Ignores are at least visible in the UI and in a REST
request (see, for example,
https://ci.ignite.apache.org/viewLog.html?buildId=2590524&tab=buildResultsDiv&buildTypeId=IgniteTests24Java8_PlatformNet&branch_IgniteTests24Java8=%3Cdefault%3E
).

Please advice me or share a link to a build with Java JUnit Ignores, which
is visible in TC.

Sincerely,
Dmitriy Pavlov

ср, 19 дек. 2018 г. в 11:59, Павлухин Иван <vololo100@gmail.com>:

> Hi Oleg,
>
> I have not quite understood who is going to call (from standpoint of
> test framework) beforeTestsStarted, beforeTest, afterTest,
> afterTestsStarted methods?
> вт, 18 дек. 2018 г. в 23:31, oignatenko <oignatenko@gridgain.com>:
> >
> > Hi Ivan,
> >
> > To answer your last question, yes, all the tests are to be marked with
> @Test
> > annotations, and those that are meant to be ignored are going to be
> marked
> > with @Ignore annotations. There is no exceptions to that, and these
> > annotations will work just as well on tests using our home brewed
> > beforeTestsStarted, beforeTest, afterTest, afterTestsStarted.
> >
> > For the sake of completeness, developers will also be able to use
> Before* /
> > After* annotations on their own methods in tests. The only exception
> > (clarified in respective javadocs) is that these annotations shouldn't be
> > used on overrides of our home brewed methods - and these methods, in
> > addition, will be recommended (not mandated) to avoid wia deprecation
> > notices.
> >
> > -----
> >
> > As for accessing tests which have problems running under junit4, the way
> how
> > I search for these in current master is regex search in IDEA for
> > "addTestSuite.*class", that is I search in testsuites for entries that
> are
> > added using method "addTestSuite" with parameter class.
> >
> > Probably worth noting that some of the problems that were previously
> > blocking addition of particular tests have been resolved in the course of
> > working on IGNITE-10177 (https://github.com/apache/ignite/pull/5615).
> One
> > riddle that currently looks particularly difficult to crack is Teamcity
> > failures in "Queries 1", I even haven't yet figured how to reproduce
> these
> > locally.
> >
> > regards, Oleg
> >
> >
> > Павлухин Иван wrote
> > > Hi Oleg,
> > >
> > > Now concerns about junit3 elimination are clear for me. And I agree
> > > that it is worth to do it. By the way is it possible to access tests
> > > which have problems running under junit4? I would like to take a look.
> > >
> > > Also a clarifying bit regarding next migration steps is needed. Sorry
> > > if it was described but I missed it. Currently we have tons of tests
> > > which rely on our home brewed beforeTestsStarted, beforeTest,
> > > afterTest, afterTestsStarted. Are you going to mark them all with
> > > corresponding junit4 annotations?
> > > пн, 17 дек. 2018 г. в 19:13, oignatenko &lt;
> >
> > > oignatenko@
> >
> > > &gt;:
> > >>
> > >> Hi Ivan,
> > >>
> > >> Per my cursory check of the code currently in master, we have 239 test
> > >> classes that couldn't migrate (that's probably about 500 or something
> > >> test
> > >> cases). For comparison, over 1600 classes migrated without problems.
> This
> > >> is
> > >> to answer your questions about how many tests are affected by change
> and
> > >> how many tests were not migrated yet.
> > >>
> > >> -----
> > >>
> > >> I think we can say that only small percent of tests turned out
> sensitive
> > >> to
> > >> JUnit framework change.
> > >>
> > >> In the same time, due to very large overall amount of our tests we
> have
> > >> quite a big number as an absolute value. I point this because if
> amount
> > >> of
> > >> troublesome tests was smaller (say, order of magnitude smaller) I
> would
> > >> consider delaying migration until all tests reworked to blend totally
> > >> seamlessly into new version JUnit. This is doable - as sort of "pilot
> > >> exercise" me and Ed adjusted one test case this way - but the sheer
> > >> amount
> > >> of work on 200+ classes raises a question, whether it is worth it (I
> > >> think
> > >> it isn't).
> > >>
> > >> With regards to question 2, changes to TestCounters are farly trivial
> > >> (and
> > >> necessary) if you look at the code diff. Prior code counted amount of
> > >> test
> > >> cases in the class by JUnit 3 convention: it picked public void
> methods
> > >> without parameters starting with "test". Changed code counts test
> cases
> > >> as
> > >> JUnit 4 does: by checking if method is annotated with @Test. Change is
> > >> necessary because it will allow test developers fully use JUnit 4
> > >> features
> > >> by adding test cases that don't follow old naming requirement. I
> > >> experimented with it a little and as far as I could see the old
> > >> TestCounters
> > >> indeed break when there are methods following new convention, it
> > >> triggered
> > >> afterTestsStopped too early.
> > >>
> > >> The answer to your question 3 lies in javadocs I added to
> runSerializer
> > >> declaration, or, more precisely, in TestCounters javadoc referred from
> > >> there. As you can see, current plan is to get rid of TestCounters
> fairly
> > >> soon (per IGNITE-10179) and this will also get rid of runSerializer so
> > >> this
> > >> is merely a temporary band aid to be removed soon.
> > >>
> > >> For the sake of completeness, my initial plan was to thoroughly
> > >> investigate
> > >> and test whether change from JUnit 3 to JUnit 4 would keep accessing
> > >> counters safe or not and only introduce runSerializer if it really
> does
> > >> but
> > >> after realising that this whole thing is likely to go away in a few
> days
> > >> from now I decided that it isn't worth wasting time and just
> temporarily
> > >> made it the way that is waterproof guaranteed to be safe.
> > >>
> > >> -----
> > >>
> > >> Now, to answer your question whether it is an option for us to keep
> part
> > >> of
> > >> tests under JUnit 3, my answer is most definitely no.
> > >>
> > >> Main reason is that having part of tests under JUnit 3 will deprive us
> > >> ability to consistently use Ignore annotation and force us fallback to
> > >> old
> > >> way to fail the tests we want to ignore. This would kind of trash the
> > >> whole
> > >> purpose of migration because we won't be able to simplify and improve
> > >> maintenance of ignored tests.
> > >>
> > >> Another important reason is that keeping JUnit 3 will much complicate
> our
> > >> test framework code. We will have to implement and maintain two
> versions
> > >> of
> > >> TestCounters (see answer to your question #2 above). We will also
> have to
> > >> keep the code that currently determines first/last test in the class
> and
> > >> possibly even complicate it to account for two versions of the
> framework
> > >> -
> > >> compare that to current plan to simply get rid of all that code per
> > >> IGNITE-10179.
> > >>
> > >> The last but not the least, this makes it much more complicated to
> later
> > >> migrate to JUnit 5. Although this is currently not in the nearest
> plans
> > >> (IGNITE-10180) we eventually will want to (especially taking into
> account
> > >> that migration from JUnit 4 is said to be easy). Having JUnit 3 tests
> > >> would
> > >> much complicate this because we have no idea if JUnit 5 could
> > >> interoperate
> > >> with such an old version (and I see no reason why we would want to
> waste
> > >> our
> > >> time and efforts investigating and testing this).
> > >>
> > >> Summing up, I believe it is very well worth it for us to get rid of
> JUnit
> > >> 3
> > >> completely.
> > >>
> > >>  -----
> > >>
> > >> With regards to making LegacySupport enabled only on purpose, at this
> > >> point
> > >> I see no reason to enforce this in code because I expect that
> deprecation
> > >> notices will do that job.
> > >>
> > >> If a developer writing new test or reworking an old one follows the
> > >> deprecation recommendations they will use JUnit 4 way instead of
> > >> deprecated
> > >> methods and you can see from the code that in this case LegacySupport
> > >> will
> > >> just transparently pass-through the test code without introducing
> > >> anything
> > >> else beyond. (Note we can reconsider and rework this later in case if
> it
> > >> turns out that my expectation doesn't hold).
> > >>
> > >> Does that answer your questions?
> > >>
> > >> regards, Oleg
> > >>
> > >>
> > >> Павлухин Иван wrote
> > >> > Hi Oleg,
> > >> >
> > >> > The things become challenging. Truly I do not see any trivial
> solution
> > >> > so far. Could you please outline main problems which we are facing
> > >> > today? And how many tests are affected? Some clarifying questions:
> > >> > 1. I know that setup->test->teardown threading was changed for
> junit4
> > >> > tests, but actually I thought that it might affect only small number
> > >> > of tests. Am I right here?
> > >> > 2. Also I saw that in your experiment [1] some changes related to
> > >> > TestCounters were made. What is wrong with them?
> > >> > 3. Why do we need wrap test execution into critical section
> > >> > (runSerializer lock)? I thought that we always run tests serially.
> > >> >
> > >> > I generally like an idea of having workaround falling back to old
> test
> > >> > execution flow. But for me the most desired trait of things like
> > >> > LegacySupport is being lightweight and enabled only on purpose. And
> > >> > from the first glance current prototype looks a little bit
> > >> > complicated. As an alternative we can keep junit3 for troublesome
> > >> > tests, can't we?
> > >> >
> > >> > Also is there any vision how many migration problems do we have so
> far
> > >> > and how many tests was not migrated yet?
> > >> > вс, 16 дек. 2018 г. в 17:39, oignatenko &lt;
> > >>
> > >> > oignatenko@
> > >>
> > >> > &gt;:
> > >> >>
> > >> >> Hi Ivan,
> > >> >>
> > >> >> As promised in my prior mail, here is the branch where I
> experimented
> > >> to
> > >> >> address concerns you raised:
> > >> >> -
> > >> >>
> > >>
> https://github.com/gridgain/apache-ignite/tree/ignite-10177-experimental
> > >> >>
> > >> >> I tested it locally and on Teamcity and it worked as intended.
> > >> >>
> > >> >> I think I managed to exactly reproduce execution sequence of JUnit
> 3
> > >> test
> > >> >> case so that tests designed expecting it will run exactly as it
was
> > >> >> before.
> > >> >>
> > >> >> As for troublesome APIs I used deprecation to warn developers
> agains
> > >> >> using
> > >> >> these and recommend what they need to use instead.
> > >> >>
> > >> >> If you are interested in closer studying the changes, class
> > >> >> GridAbstractTest1 is probably best as a starting point. This class
> is
> > >> a
> > >> >> temporary copy of GridAbstractTest made to minimise amount of
> editing
> > >> in
> > >> >> dependent classes while I was experimenting; in real implementation
> > >> (per
> > >> >> IGNITE-10177) this code is expected to be in GridAbstractTest.
> > >> >>
> > >> >> Also, I used ML testsuite to debug the changes I made, because
it
> > >> >> contains
> > >> >> convenient mix of usecases and runs fast.
> > >> >>
> > >> >> Your feedback is much appreciated.
> > >> >>
> > >> >> regards, Oleg
> > >> >>
> > >> > [...]
> > >> >>
> > >> >> --
> > >> >> Sent from: http://apache-ignite-developers.2346864.n4.nabble.com/
> > >> >
> > >> >
> > >> >
> > >> > --
> > >> > Best regards,
> > >> > Ivan Pavlukhin
> > >>
> > >>
> > >>
> > >>
> > >>
> > >> --
> > >> Sent from: http://apache-ignite-developers.2346864.n4.nabble.com/
> > >
> > >
> > >
> > > --
> > > Best regards,
> > > Ivan Pavlukhin
> >
> >
> >
> >
> >
> > --
> > Sent from: http://apache-ignite-developers.2346864.n4.nabble.com/
>
>
>
> --
> Best regards,
> Ivan Pavlukhin
>

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