Dmitriy, Thank you for noticing! It seems we have a problem here. When junit4 test is called from junit3 suite (with help of JUnit4TestAdapter) such tests is skipped silently. It seems that we cannot use @Ignore everywhere yet. ср, 19 дек. 2018 г. в 15:09, oignatenko <oignate...@gridgain.com>: > > 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/
-- Best regards, Ivan Pavlukhin
