ignite-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From oignatenko <oignate...@gridgain.com>
Subject Re: Is it time to move forward to JUnit4 (5)?
Date Wed, 19 Dec 2018 11:59:33 GMT
Hi Ivan,

These methods are called from within GridAbstractTest in exactly same
sequence as they were called in the past from JUnit 3 TestCase class.

This is by the way the reason why these methods should not be annotated with
Before* / After* - because if someone does that then method will be called
twice, once from JUnit 4 framework and one more time from GridAbstractTest.

regards, Oleg


Павлухин Иван wrote
> 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 &lt;

> oignatenko@

> &gt;:
>>
>> 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





--
Sent from: http://apache-ignite-developers.2346864.n4.nabble.com/

Mime
View raw message