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 Tue, 18 Dec 2018 20:31:15 GMT
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/

Mime
View raw message