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 < > oignatenko@ > >: >> >> 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 < >> >> > oignatenko@ >> >> > >: >> >> >> >> 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 < >> >> >> >> > oignatenko@ >> >> >> >> > >: >> >> >> >> >> >> 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/
