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/
