ignite-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Павлухин Иван <vololo...@gmail.com>
Subject Re: Is it time to move forward to JUnit4 (5)?
Date Wed, 19 Dec 2018 08:58:49 GMT
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
View raw message