Got it. Thank you.
пн, 10 дек. 2018 г. в 20:52, Dmitriy Pavlov <[hidden email]>: > Vladimir, AFAIK there will be no single change for overall migration. All > migration will be in a step-by-step manner and mostly transparent for > contributors. > > Both approaches 3&4 will be available for some time. > > First of all, we need an opportunity to run existing integration tests in > JUnit4. > > пн, 10 дек. 2018 г. в 20:38, Vladimir Ozerov <[hidden email]>: > > > Ed, > > > > Several questions from my side: > > 1) When the change is expected to be merged? > > 2) What contributors with opened PRs and new or updated JUnit3 tests are > > supposed to do? Rewrite to JUnit4? > > > > If yes, then we should give them time to have a chance to get used to new > > approach and resolve possible conflicts. > > > > Vladimir. > > > > пн, 10 дек. 2018 г. в 20:32, Eduard Shangareev < > > [hidden email] > > >: > > > > > Ivan, > > > > > > So, suggested actions with the new approach: > > > 1. Add @Test annotation on test methods. > > > 2. Add @RunWith(JUnit4.class) annotation on test class. > > > 3. Add @Before, @After on methods which should run before, after a > > > test (setUp, tearDown in current approach). > > > 4. Add your test-class to a suite using suite.addTest(new > > > JUnit4TestAdapter(YourTestClass.class)); > > > 5. Use @Ignore instead fail(); for muting test. > > > 6. You could start using @Parametrized instead of inheritance. > > > > > > > > > On Mon, Dec 3, 2018 at 1:15 PM Павлухин Иван <[hidden email]> > > wrote: > > > > > > > Hi Oleg, > > > > > > > > I noticed that GridAbstractTest is now capable to run junit4 tests. > > > > What are the current recommendations for writing new tests? Can we > use > > > > junit4 annotation for new tests? > > > > пн, 12 нояб. 2018 г. в 19:58, oignatenko <[hidden email]>: > > > > > > > > > > Hi Ivan, > > > > > > > > > > I am currently testing approach you used in pull/5354 in the > "pilot" > > > > > sub-task with examples tests (IGNITE-10174). > > > > > > > > > > So far it looks more and more like the way to go. The most > promising > > > > thing I > > > > > observed is that after I changed classes in our test framework the > > way > > > > you > > > > > did, execution of (unchanged) examples tests went exactly the same > as > > > it > > > > was > > > > > before changes. > > > > > > > > > > This indicates that existing tests won't be affected making it > indeed > > > low > > > > > risk. > > > > > > > > > > After that I converted examples tests to Junit 4 by adding @RunWith > > and > > > > > @Test annotations and tried a few, and these looked okay. > > > > > > > > > > Currently I am running full examples test suite and after it is > over > > I > > > > will > > > > > compare results to the reference list I made by running it prior to > > > > > migration. > > > > > > > > > > regards, Oleg > > > > > > > > > > > > > > > > > > > > -- > > > > > Sent from: http://apache-ignite-developers.2346864.n4.nabble.com/ > > > > > > > > > > > > > > > > -- > > > > Best regards, > > > > Ivan Pavlukhin > > > > > > > > > > |
In reply to this post by Eduard Shangareev
Hello,
I would like add few minor clarifications to what Ed wrote. 1. After transition to JUnit4 is fully over (ETA this week) there won't be a need to use @RunWith(JUnit4.class) anymore, so you will be writing test classes as a normal JUnit 4 user, without any special scaffolding. 1.1. Please keep in mind that tests that don't inherit from GridAbstractTest can (and I think should) be written full Junit 4 and don't need anything special at all - these are essentially out of scope of this discussion. 2. Note that "full JUnit 4 experience" doesn't apply to part of testsuites that currently use suite() method because in these you would still have to use JUnit4TestAdapter. Unless of course you will choose to rewrite particular suite Junit 4 style (which I would encourage everyone to do, after we complete migration). 3. As a bit of further perspective, we also considering next migration - after all tests will be uniform Junit 4 (instead of current mix of Junit 3 and 4) we will look into whether it is worth migrating to most recent version - JUnit5. Those interested can keep an eye on respective JIRA ticket: https://issues.apache.org/jira/browse/IGNITE-10180 regards, Oleg Eduard Shangareev wrote > Ivan, > > So, suggested actions with the new approach: > 1. Add @Test annotation on test methods. > 2. Add @RunWith(JUnit4.class) annotation on test class. > 3. Add @Before, @After on methods which should run before, after a > test (setUp, tearDown in current approach). > 4. Add your test-class to a suite using suite.addTest(new > JUnit4TestAdapter(YourTestClass.class)); > 5. Use @Ignore instead fail(); for muting test. > 6. You could start using @Parametrized instead of inheritance. > > > On Mon, Dec 3, 2018 at 1:15 PM Павлухин Иван < > vololo100@ > > wrote: > >> Hi Oleg, >> >> I noticed that GridAbstractTest is now capable to run junit4 tests. >> What are the current recommendations for writing new tests? Can we use >> junit4 annotation for new tests? >> пн, 12 нояб. 2018 г. в 19:58, oignatenko < > oignatenko@ > >: >> > >> > Hi Ivan, >> > >> > I am currently testing approach you used in pull/5354 in the "pilot" >> > sub-task with examples tests (IGNITE-10174). >> > >> > So far it looks more and more like the way to go. The most promising >> thing I >> > observed is that after I changed classes in our test framework the way >> you >> > did, execution of (unchanged) examples tests went exactly the same as >> it >> was >> > before changes. >> > >> > This indicates that existing tests won't be affected making it indeed >> low >> > risk. >> > >> > After that I converted examples tests to Junit 4 by adding @RunWith and >> > @Test annotations and tried a few, and these looked okay. >> > >> > Currently I am running full examples test suite and after it is over I >> will >> > compare results to the reference list I made by running it prior to >> > migration. >> > >> > 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/ |
In reply to this post by Vladimir Ozerov
Hi Vovan,
I just created JIRA ticket to address your concerns: - https://issues.apache.org/jira/browse/IGNITE-10629 In brief, the plan is that a week or two after migration is over we will run code inspection that detects JUnit 3 style tests that lack @Test annotation and fix these tests if there are any. Does that answer your question? regards, Oleg Vladimir Ozerov wrote > Ed, > > Several questions from my side: > 1) When the change is expected to be merged? > 2) What contributors with opened PRs and new or updated JUnit3 tests are > supposed to do? Rewrite to JUnit4? > > If yes, then we should give them time to have a chance to get used to new > approach and resolve possible conflicts. > > Vladimir. > > пн, 10 дек. 2018 г. в 20:32, Eduard Shangareev < > eduard.shangareev@ > >: > >> Ivan, >> >> So, suggested actions with the new approach: >> 1. Add @Test annotation on test methods. >> 2. Add @RunWith(JUnit4.class) annotation on test class. >> 3. Add @Before, @After on methods which should run before, after a >> test (setUp, tearDown in current approach). >> 4. Add your test-class to a suite using suite.addTest(new >> JUnit4TestAdapter(YourTestClass.class)); >> 5. Use @Ignore instead fail(); for muting test. >> 6. You could start using @Parametrized instead of inheritance. >> >> >> On Mon, Dec 3, 2018 at 1:15 PM Павлухин Иван < > vololo100@ > > wrote: >> >> > Hi Oleg, >> > >> > I noticed that GridAbstractTest is now capable to run junit4 tests. >> > What are the current recommendations for writing new tests? Can we use >> > junit4 annotation for new tests? >> > пн, 12 нояб. 2018 г. в 19:58, oignatenko < > oignatenko@ > >: >> > > >> > > Hi Ivan, >> > > >> > > I am currently testing approach you used in pull/5354 in the "pilot" >> > > sub-task with examples tests (IGNITE-10174). >> > > >> > > So far it looks more and more like the way to go. The most promising >> > thing I >> > > observed is that after I changed classes in our test framework the >> way >> > you >> > > did, execution of (unchanged) examples tests went exactly the same as >> it >> > was >> > > before changes. >> > > >> > > This indicates that existing tests won't be affected making it indeed >> low >> > > risk. >> > > >> > > After that I converted examples tests to Junit 4 by adding @RunWith >> and >> > > @Test annotations and tried a few, and these looked okay. >> > > >> > > Currently I am running full examples test suite and after it is over >> I >> > will >> > > compare results to the reference list I made by running it prior to >> > > migration. >> > > >> > > 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/ |
Hi Oleg,
Yes, makes perfect sense. Thank you. On Mon, Dec 10, 2018 at 10:14 PM oignatenko <[hidden email]> wrote: > Hi Vovan, > > I just created JIRA ticket to address your concerns: > - https://issues.apache.org/jira/browse/IGNITE-10629 > > In brief, the plan is that a week or two after migration is over we will > run > code inspection that detects JUnit 3 style tests that lack @Test annotation > and fix these tests if there are any. > > Does that answer your question? > > regards, Oleg > Vladimir Ozerov wrote > > Ed, > > > > Several questions from my side: > > 1) When the change is expected to be merged? > > 2) What contributors with opened PRs and new or updated JUnit3 tests are > > supposed to do? Rewrite to JUnit4? > > > > If yes, then we should give them time to have a chance to get used to new > > approach and resolve possible conflicts. > > > > Vladimir. > > > > пн, 10 дек. 2018 г. в 20:32, Eduard Shangareev < > > > eduard.shangareev@ > > > >: > > > >> Ivan, > >> > >> So, suggested actions with the new approach: > >> 1. Add @Test annotation on test methods. > >> 2. Add @RunWith(JUnit4.class) annotation on test class. > >> 3. Add @Before, @After on methods which should run before, after a > >> test (setUp, tearDown in current approach). > >> 4. Add your test-class to a suite using suite.addTest(new > >> JUnit4TestAdapter(YourTestClass.class)); > >> 5. Use @Ignore instead fail(); for muting test. > >> 6. You could start using @Parametrized instead of inheritance. > >> > >> > >> On Mon, Dec 3, 2018 at 1:15 PM Павлухин Иван < > > > vololo100@ > > > > wrote: > >> > >> > Hi Oleg, > >> > > >> > I noticed that GridAbstractTest is now capable to run junit4 tests. > >> > What are the current recommendations for writing new tests? Can we use > >> > junit4 annotation for new tests? > >> > пн, 12 нояб. 2018 г. в 19:58, oignatenko < > > > oignatenko@ > > > >: > >> > > > >> > > Hi Ivan, > >> > > > >> > > I am currently testing approach you used in pull/5354 in the "pilot" > >> > > sub-task with examples tests (IGNITE-10174). > >> > > > >> > > So far it looks more and more like the way to go. The most promising > >> > thing I > >> > > observed is that after I changed classes in our test framework the > >> way > >> > you > >> > > did, execution of (unchanged) examples tests went exactly the same > as > >> it > >> > was > >> > > before changes. > >> > > > >> > > This indicates that existing tests won't be affected making it > indeed > >> low > >> > > risk. > >> > > > >> > > After that I converted examples tests to Junit 4 by adding @RunWith > >> and > >> > > @Test annotations and tried a few, and these looked okay. > >> > > > >> > > Currently I am running full examples test suite and after it is over > >> I > >> > will > >> > > compare results to the reference list I made by running it prior to > >> > > migration. > >> > > > >> > > 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/ > |
Ed, Oleg,
Could you please clarify on the following point: > 3. Add @Before, @After on methods which should run before, after a > test (setUp, tearDown in current approach). Beforehand we used to override beforeTestsStarted, beforeTest, afterTest, afterTestsStarted. What will happen with these methods after migration to junit4? I can see that order of execution can become tricky especially when both styles are used at the same time. And an immediate suggestion is to avoid using both styles (annotations and overridden methods) in the same test. What is your insight? вт, 11 дек. 2018 г. в 10:27, Vladimir Ozerov <[hidden email]>: > > Hi Oleg, > > Yes, makes perfect sense. Thank you. > > On Mon, Dec 10, 2018 at 10:14 PM oignatenko <[hidden email]> wrote: > > > Hi Vovan, > > > > I just created JIRA ticket to address your concerns: > > - https://issues.apache.org/jira/browse/IGNITE-10629 > > > > In brief, the plan is that a week or two after migration is over we will > > run > > code inspection that detects JUnit 3 style tests that lack @Test annotation > > and fix these tests if there are any. > > > > Does that answer your question? > > > > regards, Oleg > > Vladimir Ozerov wrote > > > Ed, > > > > > > Several questions from my side: > > > 1) When the change is expected to be merged? > > > 2) What contributors with opened PRs and new or updated JUnit3 tests are > > > supposed to do? Rewrite to JUnit4? > > > > > > If yes, then we should give them time to have a chance to get used to new > > > approach and resolve possible conflicts. > > > > > > Vladimir. > > > > > > пн, 10 дек. 2018 г. в 20:32, Eduard Shangareev < > > > > > eduard.shangareev@ > > > > > >: > > > > > >> Ivan, > > >> > > >> So, suggested actions with the new approach: > > >> 1. Add @Test annotation on test methods. > > >> 2. Add @RunWith(JUnit4.class) annotation on test class. > > >> 3. Add @Before, @After on methods which should run before, after a > > >> test (setUp, tearDown in current approach). > > >> 4. Add your test-class to a suite using suite.addTest(new > > >> JUnit4TestAdapter(YourTestClass.class)); > > >> 5. Use @Ignore instead fail(); for muting test. > > >> 6. You could start using @Parametrized instead of inheritance. > > >> > > >> > > >> On Mon, Dec 3, 2018 at 1:15 PM Павлухин Иван < > > > > > vololo100@ > > > > > > wrote: > > >> > > >> > Hi Oleg, > > >> > > > >> > I noticed that GridAbstractTest is now capable to run junit4 tests. > > >> > What are the current recommendations for writing new tests? Can we use > > >> > junit4 annotation for new tests? > > >> > пн, 12 нояб. 2018 г. в 19:58, oignatenko < > > > > > oignatenko@ > > > > > >: > > >> > > > > >> > > Hi Ivan, > > >> > > > > >> > > I am currently testing approach you used in pull/5354 in the "pilot" > > >> > > sub-task with examples tests (IGNITE-10174). > > >> > > > > >> > > So far it looks more and more like the way to go. The most promising > > >> > thing I > > >> > > observed is that after I changed classes in our test framework the > > >> way > > >> > you > > >> > > did, execution of (unchanged) examples tests went exactly the same > > as > > >> it > > >> > was > > >> > > before changes. > > >> > > > > >> > > This indicates that existing tests won't be affected making it > > indeed > > >> low > > >> > > risk. > > >> > > > > >> > > After that I converted examples tests to Junit 4 by adding @RunWith > > >> and > > >> > > @Test annotations and tried a few, and these looked okay. > > >> > > > > >> > > Currently I am running full examples test suite and after it is over > > >> I > > >> > will > > >> > > compare results to the reference list I made by running it prior to > > >> > > migration. > > >> > > > > >> > > 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 |
Hi Ivan,
That's a very good question and I think you spotted a point where Ignite test developer's preferences need to change when migrating from Junit 3 to 4. In brief, when developing under JUnit 3 there were good reasons to prefer our homebrew template methods (beforeTestsStarted, beforeTest, afterTest, afterTestsStopped) over those provided by JUnit (setUp / tearDown). To answer your other question, in order to reliably understand how our methods work one would better go to (potentially changing) code of GridAbstractTest and check where and how these are invoked. But under JUnit 4 these reasons don't apply anymore and preference should go the other way, namely in favor of annotations @Before, @After, @BeforeClass, @AfterClass. In other words, when these annotations allow you implement desired test logic you better use them. These annotations have stable, well documented and widely understood semantics (and don't require one to skim through 3K lines of code for grasping details:). And it makes sense to "fallback" to our old-fashioned methods only when you simply have no other choice. For example doing some quick and simple fix in a legacy code that is filled with multiple usages of our old template methods it may be okay to choose to keep old style - instead of doing cumbersome, risky and lengthy refactoring or polluting test code with inconsistent use of annotations intertwined with old style methods. That's what I think. If you're interested in why I think so, refer to boring explanation in PS. regards, Oleg ----- PS. With regards to old preferences I believe it is very important to keep in mind that Junit 3 didn't provide means to do initialization and cleanup once for all test methods in the class. So when one needed things to run once per test class they had to go somewhere else. In Ignite the natural way to go was GridAbstractTest which provided this functionality via beforeTestsStarted and afterTestsStopped (with some glitches as I recently learned but still). And after one had to use these methods there was no compelling reason to use JUnit's setUp and tearDown, because GridAbstractTest also provided similar methods. It was just easier to consistently use full and cohesive set of methods provided by GridAbstractTest than switch back and forth between it and JUnit 3 methods. With JUnit 4 above reasoning doesn't apply anymore because its features are just as full and cohesive and can be consistently used without having to fallback to stuff from GridAbstractTest. So we have a real choice now and can judge based on other criteria than functional completeness (since both ways now satisfy that most important criteria). And this reveals us a full spectrum of JUnit advantages over GridAbstractTest (which was obscured before, because JUnit 3 was functionally incomplete). And on that newly opened spectrum JUnit 4 API wins hands down. JUnit 4 is much better documented and "googlable", it is stable and much wider known. The last but not the least, as I already mentioned, it doesn't require one to skim through 3K lines of code for grasping details and its semantics doesn't depend on (possibly changing) implementation code. Also, initialization and cleanup methods of JUnit 4 make an organic part of wider, more comprehensive API which smoothly integrates with IDE, Maven, Teamcity and all imaginable Java tools over there. Every time one annotates @Test or @Ignore gives them a reason to prefer other JUnit annotations over something else. It works about the same as I described above for beforeTestsStarted in GridAbstractTest and how it inhibited use of JUnit 3 methods: after you picked one part of some API it naturally becomes more convenient to keep using other parts of that API. Summing up, I believe that every time you have a (sensible) choice between JUnit 4 and GridAbstractTest methods you just pick JUnit 4 and don't look back. Павлухин Иван wrote > Ed, Oleg, > > Could you please clarify on the following point: >> 3. Add @Before, @After on methods which should run before, after a >> test (setUp, tearDown in current approach). > Beforehand we used to override beforeTestsStarted, beforeTest, > afterTest, afterTestsStarted. What will happen with these methods > after migration to junit4? I can see that order of execution can > become tricky especially when both styles are used at the same time. > And an immediate suggestion is to avoid using both styles (annotations > and overridden methods) in the same test. What is your insight? > вт, 11 дек. 2018 г. в 10:27, Vladimir Ozerov < > vozerov@ > >: >> >> Hi Oleg, >> >> Yes, makes perfect sense. Thank you. >> >> On Mon, Dec 10, 2018 at 10:14 PM oignatenko < > oignatenko@ > > wrote: >> >> > Hi Vovan, >> > >> > I just created JIRA ticket to address your concerns: >> > - https://issues.apache.org/jira/browse/IGNITE-10629 >> > >> > In brief, the plan is that a week or two after migration is over we >> will >> > run >> > code inspection that detects JUnit 3 style tests that lack @Test >> annotation >> > and fix these tests if there are any. >> > >> > Does that answer your question? >> > >> > regards, Oleg >> > Vladimir Ozerov wrote >> > > Ed, >> > > >> > > Several questions from my side: >> > > 1) When the change is expected to be merged? >> > > 2) What contributors with opened PRs and new or updated JUnit3 tests >> are >> > > supposed to do? Rewrite to JUnit4? >> > > >> > > If yes, then we should give them time to have a chance to get used to >> new >> > > approach and resolve possible conflicts. >> > > >> > > Vladimir. >> > > >> > > пн, 10 дек. 2018 г. в 20:32, Eduard Shangareev < >> > >> > > eduard.shangareev@ >> > >> > > >: >> > > >> > >> Ivan, >> > >> >> > >> So, suggested actions with the new approach: >> > >> 1. Add @Test annotation on test methods. >> > >> 2. Add @RunWith(JUnit4.class) annotation on test class. >> > >> 3. Add @Before, @After on methods which should run before, after a >> > >> test (setUp, tearDown in current approach). >> > >> 4. Add your test-class to a suite using suite.addTest(new >> > >> JUnit4TestAdapter(YourTestClass.class)); >> > >> 5. Use @Ignore instead fail(); for muting test. >> > >> 6. You could start using @Parametrized instead of inheritance. >> > >> >> > >> >> > >> On Mon, Dec 3, 2018 at 1:15 PM Павлухин Иван < >> > >> > > vololo100@ >> > >> > > > wrote: >> > >> >> > >> > Hi Oleg, >> > >> > >> > >> > I noticed that GridAbstractTest is now capable to run junit4 >> tests. >> > >> > What are the current recommendations for writing new tests? Can we >> use >> > >> > junit4 annotation for new tests? >> > >> > пн, 12 нояб. 2018 г. в 19:58, oignatenko < >> > >> > > oignatenko@ >> > >> > > >: >> > >> > > >> > >> > > Hi Ivan, >> > >> > > >> > >> > > I am currently testing approach you used in pull/5354 in the >> "pilot" >> > >> > > sub-task with examples tests (IGNITE-10174). >> > >> > > >> > >> > > So far it looks more and more like the way to go. The most >> promising >> > >> > thing I >> > >> > > observed is that after I changed classes in our test framework >> the >> > >> way >> > >> > you >> > >> > > did, execution of (unchanged) examples tests went exactly the >> same >> > as >> > >> it >> > >> > was >> > >> > > before changes. >> > >> > > >> > >> > > This indicates that existing tests won't be affected making it >> > indeed >> > >> low >> > >> > > risk. >> > >> > > >> > >> > > After that I converted examples tests to Junit 4 by adding >> @RunWith >> > >> and >> > >> > > @Test annotations and tried a few, and these looked okay. >> > >> > > >> > >> > > Currently I am running full examples test suite and after it is >> over >> > >> I >> > >> > will >> > >> > > compare results to the reference list I made by running it prior >> to >> > >> > > migration. >> > >> > > >> > >> > > 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/ |
Hi Oleg,
Thanks your for detailed answer. Using junit4 annotations where possible sounds good to me. Also I would like to thank everybody involved in migration to junit4 process for your efforts. You were able to move a problem which looked mountain-heavy. I believe that is a significant milestone on Ignite stabilization road. Well done! вт, 11 дек. 2018 г. в 20:20, oignatenko <[hidden email]>: > > Hi Ivan, > > That's a very good question and I think you spotted a point where Ignite > test developer's preferences need to change when migrating from Junit 3 to > 4. > > In brief, when developing under JUnit 3 there were good reasons to prefer > our homebrew template methods (beforeTestsStarted, beforeTest, afterTest, > afterTestsStopped) over those provided by JUnit (setUp / tearDown). To > answer your other question, in order to reliably understand how our methods > work one would better go to (potentially changing) code of GridAbstractTest > and check where and how these are invoked. > > But under JUnit 4 these reasons don't apply anymore and preference should go > the other way, namely in favor of annotations @Before, @After, @BeforeClass, > @AfterClass. > > In other words, when these annotations allow you implement desired test > logic you better use them. These annotations have stable, well documented > and widely understood semantics (and don't require one to skim through 3K > lines of code for grasping details:). > > And it makes sense to "fallback" to our old-fashioned methods only when you > simply have no other choice. For example doing some quick and simple fix in > a legacy code that is filled with multiple usages of our old template > methods it may be okay to choose to keep old style - instead of doing > cumbersome, risky and lengthy refactoring or polluting test code with > inconsistent use of annotations intertwined with old style methods. > > That's what I think. If you're interested in why I think so, refer to boring > explanation in PS. > > regards, Oleg > > ----- > > PS. With regards to old preferences I believe it is very important to keep > in mind that Junit 3 didn't provide means to do initialization and cleanup > once for all test methods in the class. > > So when one needed things to run once per test class they had to go > somewhere else. In Ignite the natural way to go was GridAbstractTest which > provided this functionality via beforeTestsStarted and afterTestsStopped > (with some glitches as I recently learned but still). And after one had to > use these methods there was no compelling reason to use JUnit's setUp and > tearDown, because GridAbstractTest also provided similar methods. > > It was just easier to consistently use full and cohesive set of methods > provided by GridAbstractTest than switch back and forth between it and JUnit > 3 methods. > > With JUnit 4 above reasoning doesn't apply anymore because its features are > just as full and cohesive and can be consistently used without having to > fallback to stuff from GridAbstractTest. So we have a real choice now and > can judge based on other criteria than functional completeness (since both > ways now satisfy that most important criteria). > > And this reveals us a full spectrum of JUnit advantages over > GridAbstractTest (which was obscured before, because JUnit 3 was > functionally incomplete). > > And on that newly opened spectrum JUnit 4 API wins hands down. > > JUnit 4 is much better documented and "googlable", it is stable and much > wider known. The last but not the least, as I already mentioned, it doesn't > require one to skim through 3K lines of code for grasping details and its > semantics doesn't depend on (possibly changing) implementation code. > > Also, initialization and cleanup methods of JUnit 4 make an organic part of > wider, more comprehensive API which smoothly integrates with IDE, Maven, > Teamcity and all imaginable Java tools over there. > > Every time one annotates @Test or @Ignore gives them a reason to prefer > other JUnit annotations over something else. It works about the same as I > described above for beforeTestsStarted in GridAbstractTest and how it > inhibited use of JUnit 3 methods: after you picked one part of some API it > naturally becomes more convenient to keep using other parts of that API. > > Summing up, I believe that every time you have a (sensible) choice between > JUnit 4 and GridAbstractTest methods you just pick JUnit 4 and don't look > back. > > > Павлухин Иван wrote > > Ed, Oleg, > > > > Could you please clarify on the following point: > >> 3. Add @Before, @After on methods which should run before, after a > >> test (setUp, tearDown in current approach). > > Beforehand we used to override beforeTestsStarted, beforeTest, > > afterTest, afterTestsStarted. What will happen with these methods > > after migration to junit4? I can see that order of execution can > > become tricky especially when both styles are used at the same time. > > And an immediate suggestion is to avoid using both styles (annotations > > and overridden methods) in the same test. What is your insight? > > вт, 11 дек. 2018 г. в 10:27, Vladimir Ozerov < > > > vozerov@ > > > >: > >> > >> Hi Oleg, > >> > >> Yes, makes perfect sense. Thank you. > >> > >> On Mon, Dec 10, 2018 at 10:14 PM oignatenko < > > > oignatenko@ > > > > wrote: > >> > >> > Hi Vovan, > >> > > >> > I just created JIRA ticket to address your concerns: > >> > - https://issues.apache.org/jira/browse/IGNITE-10629 > >> > > >> > In brief, the plan is that a week or two after migration is over we > >> will > >> > run > >> > code inspection that detects JUnit 3 style tests that lack @Test > >> annotation > >> > and fix these tests if there are any. > >> > > >> > Does that answer your question? > >> > > >> > regards, Oleg > >> > Vladimir Ozerov wrote > >> > > Ed, > >> > > > >> > > Several questions from my side: > >> > > 1) When the change is expected to be merged? > >> > > 2) What contributors with opened PRs and new or updated JUnit3 tests > >> are > >> > > supposed to do? Rewrite to JUnit4? > >> > > > >> > > If yes, then we should give them time to have a chance to get used to > >> new > >> > > approach and resolve possible conflicts. > >> > > > >> > > Vladimir. > >> > > > >> > > пн, 10 дек. 2018 г. в 20:32, Eduard Shangareev < > >> > > >> > > eduard.shangareev@ > >> > > >> > > >: > >> > > > >> > >> Ivan, > >> > >> > >> > >> So, suggested actions with the new approach: > >> > >> 1. Add @Test annotation on test methods. > >> > >> 2. Add @RunWith(JUnit4.class) annotation on test class. > >> > >> 3. Add @Before, @After on methods which should run before, after a > >> > >> test (setUp, tearDown in current approach). > >> > >> 4. Add your test-class to a suite using suite.addTest(new > >> > >> JUnit4TestAdapter(YourTestClass.class)); > >> > >> 5. Use @Ignore instead fail(); for muting test. > >> > >> 6. You could start using @Parametrized instead of inheritance. > >> > >> > >> > >> > >> > >> On Mon, Dec 3, 2018 at 1:15 PM Павлухин Иван < > >> > > >> > > vololo100@ > >> > > >> > > > wrote: > >> > >> > >> > >> > Hi Oleg, > >> > >> > > >> > >> > I noticed that GridAbstractTest is now capable to run junit4 > >> tests. > >> > >> > What are the current recommendations for writing new tests? Can we > >> use > >> > >> > junit4 annotation for new tests? > >> > >> > пн, 12 нояб. 2018 г. в 19:58, oignatenko < > >> > > >> > > oignatenko@ > >> > > >> > > >: > >> > >> > > > >> > >> > > Hi Ivan, > >> > >> > > > >> > >> > > I am currently testing approach you used in pull/5354 in the > >> "pilot" > >> > >> > > sub-task with examples tests (IGNITE-10174). > >> > >> > > > >> > >> > > So far it looks more and more like the way to go. The most > >> promising > >> > >> > thing I > >> > >> > > observed is that after I changed classes in our test framework > >> the > >> > >> way > >> > >> > you > >> > >> > > did, execution of (unchanged) examples tests went exactly the > >> same > >> > as > >> > >> it > >> > >> > was > >> > >> > > before changes. > >> > >> > > > >> > >> > > This indicates that existing tests won't be affected making it > >> > indeed > >> > >> low > >> > >> > > risk. > >> > >> > > > >> > >> > > After that I converted examples tests to Junit 4 by adding > >> @RunWith > >> > >> and > >> > >> > > @Test annotations and tried a few, and these looked okay. > >> > >> > > > >> > >> > > Currently I am running full examples test suite and after it is > >> over > >> > >> I > >> > >> > will > >> > >> > > compare results to the reference list I made by running it prior > >> to > >> > >> > > migration. > >> > >> > > > >> > >> > > 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 |
Hi guys,
I started writing a junit4 test for a feature I am currently working on. And immediately I faced a peculiarity with @Before test fixture. I automatically named a method "setUp" and realized that I override a method from a superclass. It can produce unwanted effects. First, it looks reasonable to call a corresponding superclass method in an overridden method, but in my understanding it is not a canonical way in junit4. Second, in junit4 overriding method annotated with @Before can lead changing order of this method execution among other @Before method. Second point might lead to surprising effects in tests hierarchies defining @Before methods on different levels. Let's return to the test I mentioned in the beginning of current email. I decided to use a different name for my @Before method (actually it was unpleasant "setUp1"). I think that we can do the following to make things straightforward: 1. Prohibit overriding base class setUp and tearDown methods by making them final (at least in GridCommonAbstractTest and GridSpiAbstractTest). 2. Use common default name for @Before and @After methods defined in concrete test classes. I do not have really good candidates for names. Simply we can use setUp1, tearDown1. Another option initTest, cleanupTest. Your name candidates are quite appreciated. WDYT? P.S. Also I went through other tests and found really dangerous thing. In IgniteJmsStreamerTest (not migrated to junit4 yet) we have beforeTest method (overriding GridAbstractTest framework method) annotated with @Before annotation (I believe accidentally)! It means that migrating such test to junit4 will lead to calling beforeTest twice. Ouch! ср, 12 дек. 2018 г. в 13:21, Павлухин Иван <[hidden email]>: > > Hi Oleg, > > Thanks your for detailed answer. Using junit4 annotations where > possible sounds good to me. > > Also I would like to thank everybody involved in migration to junit4 > process for your efforts. You were able to move a problem which looked > mountain-heavy. I believe that is a significant milestone on Ignite > stabilization road. > > Well done! > вт, 11 дек. 2018 г. в 20:20, oignatenko <[hidden email]>: > > > > Hi Ivan, > > > > That's a very good question and I think you spotted a point where Ignite > > test developer's preferences need to change when migrating from Junit 3 to > > 4. > > > > In brief, when developing under JUnit 3 there were good reasons to prefer > > our homebrew template methods (beforeTestsStarted, beforeTest, afterTest, > > afterTestsStopped) over those provided by JUnit (setUp / tearDown). To > > answer your other question, in order to reliably understand how our methods > > work one would better go to (potentially changing) code of GridAbstractTest > > and check where and how these are invoked. > > > > But under JUnit 4 these reasons don't apply anymore and preference should go > > the other way, namely in favor of annotations @Before, @After, @BeforeClass, > > @AfterClass. > > > > In other words, when these annotations allow you implement desired test > > logic you better use them. These annotations have stable, well documented > > and widely understood semantics (and don't require one to skim through 3K > > lines of code for grasping details:). > > > > And it makes sense to "fallback" to our old-fashioned methods only when you > > simply have no other choice. For example doing some quick and simple fix in > > a legacy code that is filled with multiple usages of our old template > > methods it may be okay to choose to keep old style - instead of doing > > cumbersome, risky and lengthy refactoring or polluting test code with > > inconsistent use of annotations intertwined with old style methods. > > > > That's what I think. If you're interested in why I think so, refer to boring > > explanation in PS. > > > > regards, Oleg > > > > ----- > > > > PS. With regards to old preferences I believe it is very important to keep > > in mind that Junit 3 didn't provide means to do initialization and cleanup > > once for all test methods in the class. > > > > So when one needed things to run once per test class they had to go > > somewhere else. In Ignite the natural way to go was GridAbstractTest which > > provided this functionality via beforeTestsStarted and afterTestsStopped > > (with some glitches as I recently learned but still). And after one had to > > use these methods there was no compelling reason to use JUnit's setUp and > > tearDown, because GridAbstractTest also provided similar methods. > > > > It was just easier to consistently use full and cohesive set of methods > > provided by GridAbstractTest than switch back and forth between it and JUnit > > 3 methods. > > > > With JUnit 4 above reasoning doesn't apply anymore because its features are > > just as full and cohesive and can be consistently used without having to > > fallback to stuff from GridAbstractTest. So we have a real choice now and > > can judge based on other criteria than functional completeness (since both > > ways now satisfy that most important criteria). > > > > And this reveals us a full spectrum of JUnit advantages over > > GridAbstractTest (which was obscured before, because JUnit 3 was > > functionally incomplete). > > > > And on that newly opened spectrum JUnit 4 API wins hands down. > > > > JUnit 4 is much better documented and "googlable", it is stable and much > > wider known. The last but not the least, as I already mentioned, it doesn't > > require one to skim through 3K lines of code for grasping details and its > > semantics doesn't depend on (possibly changing) implementation code. > > > > Also, initialization and cleanup methods of JUnit 4 make an organic part of > > wider, more comprehensive API which smoothly integrates with IDE, Maven, > > Teamcity and all imaginable Java tools over there. > > > > Every time one annotates @Test or @Ignore gives them a reason to prefer > > other JUnit annotations over something else. It works about the same as I > > described above for beforeTestsStarted in GridAbstractTest and how it > > inhibited use of JUnit 3 methods: after you picked one part of some API it > > naturally becomes more convenient to keep using other parts of that API. > > > > Summing up, I believe that every time you have a (sensible) choice between > > JUnit 4 and GridAbstractTest methods you just pick JUnit 4 and don't look > > back. > > > > > > Павлухин Иван wrote > > > Ed, Oleg, > > > > > > Could you please clarify on the following point: > > >> 3. Add @Before, @After on methods which should run before, after a > > >> test (setUp, tearDown in current approach). > > > Beforehand we used to override beforeTestsStarted, beforeTest, > > > afterTest, afterTestsStarted. What will happen with these methods > > > after migration to junit4? I can see that order of execution can > > > become tricky especially when both styles are used at the same time. > > > And an immediate suggestion is to avoid using both styles (annotations > > > and overridden methods) in the same test. What is your insight? > > > вт, 11 дек. 2018 г. в 10:27, Vladimir Ozerov < > > > > > vozerov@ > > > > > >: > > >> > > >> Hi Oleg, > > >> > > >> Yes, makes perfect sense. Thank you. > > >> > > >> On Mon, Dec 10, 2018 at 10:14 PM oignatenko < > > > > > oignatenko@ > > > > > > wrote: > > >> > > >> > Hi Vovan, > > >> > > > >> > I just created JIRA ticket to address your concerns: > > >> > - https://issues.apache.org/jira/browse/IGNITE-10629 > > >> > > > >> > In brief, the plan is that a week or two after migration is over we > > >> will > > >> > run > > >> > code inspection that detects JUnit 3 style tests that lack @Test > > >> annotation > > >> > and fix these tests if there are any. > > >> > > > >> > Does that answer your question? > > >> > > > >> > regards, Oleg > > >> > Vladimir Ozerov wrote > > >> > > Ed, > > >> > > > > >> > > Several questions from my side: > > >> > > 1) When the change is expected to be merged? > > >> > > 2) What contributors with opened PRs and new or updated JUnit3 tests > > >> are > > >> > > supposed to do? Rewrite to JUnit4? > > >> > > > > >> > > If yes, then we should give them time to have a chance to get used to > > >> new > > >> > > approach and resolve possible conflicts. > > >> > > > > >> > > Vladimir. > > >> > > > > >> > > пн, 10 дек. 2018 г. в 20:32, Eduard Shangareev < > > >> > > > >> > > eduard.shangareev@ > > >> > > > >> > > >: > > >> > > > > >> > >> Ivan, > > >> > >> > > >> > >> So, suggested actions with the new approach: > > >> > >> 1. Add @Test annotation on test methods. > > >> > >> 2. Add @RunWith(JUnit4.class) annotation on test class. > > >> > >> 3. Add @Before, @After on methods which should run before, after a > > >> > >> test (setUp, tearDown in current approach). > > >> > >> 4. Add your test-class to a suite using suite.addTest(new > > >> > >> JUnit4TestAdapter(YourTestClass.class)); > > >> > >> 5. Use @Ignore instead fail(); for muting test. > > >> > >> 6. You could start using @Parametrized instead of inheritance. > > >> > >> > > >> > >> > > >> > >> On Mon, Dec 3, 2018 at 1:15 PM Павлухин Иван < > > >> > > > >> > > vololo100@ > > >> > > > >> > > > wrote: > > >> > >> > > >> > >> > Hi Oleg, > > >> > >> > > > >> > >> > I noticed that GridAbstractTest is now capable to run junit4 > > >> tests. > > >> > >> > What are the current recommendations for writing new tests? Can we > > >> use > > >> > >> > junit4 annotation for new tests? > > >> > >> > пн, 12 нояб. 2018 г. в 19:58, oignatenko < > > >> > > > >> > > oignatenko@ > > >> > > > >> > > >: > > >> > >> > > > > >> > >> > > Hi Ivan, > > >> > >> > > > > >> > >> > > I am currently testing approach you used in pull/5354 in the > > >> "pilot" > > >> > >> > > sub-task with examples tests (IGNITE-10174). > > >> > >> > > > > >> > >> > > So far it looks more and more like the way to go. The most > > >> promising > > >> > >> > thing I > > >> > >> > > observed is that after I changed classes in our test framework > > >> the > > >> > >> way > > >> > >> > you > > >> > >> > > did, execution of (unchanged) examples tests went exactly the > > >> same > > >> > as > > >> > >> it > > >> > >> > was > > >> > >> > > before changes. > > >> > >> > > > > >> > >> > > This indicates that existing tests won't be affected making it > > >> > indeed > > >> > >> low > > >> > >> > > risk. > > >> > >> > > > > >> > >> > > After that I converted examples tests to Junit 4 by adding > > >> @RunWith > > >> > >> and > > >> > >> > > @Test annotations and tried a few, and these looked okay. > > >> > >> > > > > >> > >> > > Currently I am running full examples test suite and after it is > > >> over > > >> > >> I > > >> > >> > will > > >> > >> > > compare results to the reference list I made by running it prior > > >> to > > >> > >> > > migration. > > >> > >> > > > > >> > >> > > 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 -- Best regards, Ivan Pavlukhin |
Hi Ivan,
The issues you mentioned are real and these issues are expected to be addressed per IGNITE-10177. I am currently experimenting with various approaches to tackle that, will post here when "early preview implementation" is ready to let others take a look and test it. Until this is completed the most reliable approach I've found is to fallback to JUnit 3 way every time that you observe unexpected test behavior and see no reasonable way to fix it. You can see how I followed this "transitional" approach in IGNITE-10176 which is expected to be merged to master soon (meanwhile the code can be seen in PR https://github.com/apache/ignite/pull/5659). ----- With regards to using Before* and After* annotations on overrides of before* and after* methods of GridAbstractTest, this is pure evil and should be always avoided (removed if noticed). When test runs under JUnit 3 these annotations are simply ignored which makes them totally useless (and misleading to the reader of the code) and when it runs under JUnit 4 these start doing real harm by making annotated method run twice. If you are interested, more detailed explanation with code example is available in comments under IGNITE-10176: - https://issues.apache.org/jira/browse/IGNITE-10176?focusedCommentId=16718914&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-16718914 I am currently trying to figure what could be the best way to communicate this to API users, possibly in respective method javadocs. Also I plan to find and clean up all the inappropriate annotations that are already there in the project in the IGNITE-10177. If you have further questions, please let me know. regards, Oleg -- Sent from: http://apache-ignite-developers.2346864.n4.nabble.com/ |
In reply to this post by Ivan Pavlukhin
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 Павлухин Иван wrote > Hi guys, > > I started writing a junit4 test for a feature I am currently working > on. And immediately I faced a peculiarity with @Before test fixture. I > automatically named a method "setUp" and realized that I override a > method from a superclass. It can produce unwanted effects. First, it > looks reasonable to call a corresponding superclass method in an > overridden method, but in my understanding it is not a canonical way > in junit4. Second, in junit4 overriding method annotated with @Before > can lead changing order of this method execution among other @Before > method. Second point might lead to surprising effects in tests > hierarchies defining @Before methods on different levels. > > Let's return to the test I mentioned in the beginning of current > email. I decided to use a different name for my @Before method > (actually it was unpleasant "setUp1"). I think that we can do the > following to make things straightforward: > 1. Prohibit overriding base class setUp and tearDown methods by making > them final (at least in GridCommonAbstractTest and > GridSpiAbstractTest). > 2. Use common default name for @Before and @After methods defined in > concrete test classes. I do not have really good candidates for names. > Simply we can use setUp1, tearDown1. Another option initTest, > cleanupTest. Your name candidates are quite appreciated. > > WDYT? > > P.S. Also I went through other tests and found really dangerous thing. > In IgniteJmsStreamerTest (not migrated to junit4 yet) we have > beforeTest method (overriding GridAbstractTest framework method) > annotated with @Before annotation (I believe accidentally)! It means > that migrating such test to junit4 will lead to calling beforeTest > twice. Ouch! > ср, 12 дек. 2018 г. в 13:21, Павлухин Иван < > vololo100@ > >: >> >> Hi Oleg, >> >> Thanks your for detailed answer. Using junit4 annotations where >> possible sounds good to me. >> >> Also I would like to thank everybody involved in migration to junit4 >> process for your efforts. You were able to move a problem which looked >> mountain-heavy. I believe that is a significant milestone on Ignite >> stabilization road. >> >> Well done! >> вт, 11 дек. 2018 г. в 20:20, oignatenko < > oignatenko@ > >: >> > >> > Hi Ivan, >> > >> > That's a very good question and I think you spotted a point where >> Ignite >> > test developer's preferences need to change when migrating from Junit 3 >> to >> > 4. >> > >> > In brief, when developing under JUnit 3 there were good reasons to >> prefer >> > our homebrew template methods (beforeTestsStarted, beforeTest, >> afterTest, >> > afterTestsStopped) over those provided by JUnit (setUp / tearDown). To >> > answer your other question, in order to reliably understand how our >> methods >> > work one would better go to (potentially changing) code of >> GridAbstractTest >> > and check where and how these are invoked. >> > >> > But under JUnit 4 these reasons don't apply anymore and preference >> should go >> > the other way, namely in favor of annotations @Before, @After, >> @BeforeClass, >> > @AfterClass. >> > >> > In other words, when these annotations allow you implement desired test >> > logic you better use them. These annotations have stable, well >> documented >> > and widely understood semantics (and don't require one to skim through >> 3K >> > lines of code for grasping details:). >> > >> > And it makes sense to "fallback" to our old-fashioned methods only when >> you >> > simply have no other choice. For example doing some quick and simple >> fix in >> > a legacy code that is filled with multiple usages of our old template >> > methods it may be okay to choose to keep old style - instead of doing >> > cumbersome, risky and lengthy refactoring or polluting test code with >> > inconsistent use of annotations intertwined with old style methods. >> > >> > That's what I think. If you're interested in why I think so, refer to >> boring >> > explanation in PS. >> > >> > regards, Oleg >> > >> > ----- >> > >> > PS. With regards to old preferences I believe it is very important to >> keep >> > in mind that Junit 3 didn't provide means to do initialization and >> cleanup >> > once for all test methods in the class. >> > >> > So when one needed things to run once per test class they had to go >> > somewhere else. In Ignite the natural way to go was GridAbstractTest >> which >> > provided this functionality via beforeTestsStarted and >> afterTestsStopped >> > (with some glitches as I recently learned but still). And after one had >> to >> > use these methods there was no compelling reason to use JUnit's setUp >> and >> > tearDown, because GridAbstractTest also provided similar methods. >> > >> > It was just easier to consistently use full and cohesive set of methods >> > provided by GridAbstractTest than switch back and forth between it and >> JUnit >> > 3 methods. >> > >> > With JUnit 4 above reasoning doesn't apply anymore because its features >> are >> > just as full and cohesive and can be consistently used without having >> to >> > fallback to stuff from GridAbstractTest. So we have a real choice now >> and >> > can judge based on other criteria than functional completeness (since >> both >> > ways now satisfy that most important criteria). >> > >> > And this reveals us a full spectrum of JUnit advantages over >> > GridAbstractTest (which was obscured before, because JUnit 3 was >> > functionally incomplete). >> > >> > And on that newly opened spectrum JUnit 4 API wins hands down. >> > >> > JUnit 4 is much better documented and "googlable", it is stable and >> much >> > wider known. The last but not the least, as I already mentioned, it >> doesn't >> > require one to skim through 3K lines of code for grasping details and >> its >> > semantics doesn't depend on (possibly changing) implementation code. >> > >> > Also, initialization and cleanup methods of JUnit 4 make an organic >> part of >> > wider, more comprehensive API which smoothly integrates with IDE, >> Maven, >> > Teamcity and all imaginable Java tools over there. >> > >> > Every time one annotates @Test or @Ignore gives them a reason to prefer >> > other JUnit annotations over something else. It works about the same as >> I >> > described above for beforeTestsStarted in GridAbstractTest and how it >> > inhibited use of JUnit 3 methods: after you picked one part of some API >> it >> > naturally becomes more convenient to keep using other parts of that >> API. >> > >> > Summing up, I believe that every time you have a (sensible) choice >> between >> > JUnit 4 and GridAbstractTest methods you just pick JUnit 4 and don't >> look >> > back. >> > >> > >> > Павлухин Иван wrote >> > > Ed, Oleg, >> > > >> > > Could you please clarify on the following point: >> > >> 3. Add @Before, @After on methods which should run before, after a >> > >> test (setUp, tearDown in current approach). >> > > Beforehand we used to override beforeTestsStarted, beforeTest, >> > > afterTest, afterTestsStarted. What will happen with these methods >> > > after migration to junit4? I can see that order of execution can >> > > become tricky especially when both styles are used at the same time. >> > > And an immediate suggestion is to avoid using both styles >> (annotations >> > > and overridden methods) in the same test. What is your insight? >> > > вт, 11 дек. 2018 г. в 10:27, Vladimir Ozerov < >> > >> > > vozerov@ >> > >> > > >: >> > >> >> > >> Hi Oleg, >> > >> >> > >> Yes, makes perfect sense. Thank you. >> > >> >> > >> On Mon, Dec 10, 2018 at 10:14 PM oignatenko < >> > >> > > oignatenko@ >> > >> > > > wrote: >> > >> >> > >> > Hi Vovan, >> > >> > >> > >> > I just created JIRA ticket to address your concerns: >> > >> > - https://issues.apache.org/jira/browse/IGNITE-10629 >> > >> > >> > >> > In brief, the plan is that a week or two after migration is over >> we >> > >> will >> > >> > run >> > >> > code inspection that detects JUnit 3 style tests that lack @Test >> > >> annotation >> > >> > and fix these tests if there are any. >> > >> > >> > >> > Does that answer your question? >> > >> > >> > >> > regards, Oleg >> > >> > Vladimir Ozerov wrote >> > >> > > Ed, >> > >> > > >> > >> > > Several questions from my side: >> > >> > > 1) When the change is expected to be merged? >> > >> > > 2) What contributors with opened PRs and new or updated JUnit3 >> tests >> > >> are >> > >> > > supposed to do? Rewrite to JUnit4? >> > >> > > >> > >> > > If yes, then we should give them time to have a chance to get >> used to >> > >> new >> > >> > > approach and resolve possible conflicts. >> > >> > > >> > >> > > Vladimir. >> > >> > > >> > >> > > пн, 10 дек. 2018 г. в 20:32, Eduard Shangareev < >> > >> > >> > >> > > eduard.shangareev@ >> > >> > >> > >> > > >: >> > >> > > >> > >> > >> Ivan, >> > >> > >> >> > >> > >> So, suggested actions with the new approach: >> > >> > >> 1. Add @Test annotation on test methods. >> > >> > >> 2. Add @RunWith(JUnit4.class) annotation on test class. >> > >> > >> 3. Add @Before, @After on methods which should run before, >> after a >> > >> > >> test (setUp, tearDown in current approach). >> > >> > >> 4. Add your test-class to a suite using suite.addTest(new >> > >> > >> JUnit4TestAdapter(YourTestClass.class)); >> > >> > >> 5. Use @Ignore instead fail(); for muting test. >> > >> > >> 6. You could start using @Parametrized instead of inheritance. >> > >> > >> >> > >> > >> >> > >> > >> On Mon, Dec 3, 2018 at 1:15 PM Павлухин Иван < >> > >> > >> > >> > > vololo100@ >> > >> > >> > >> > > > wrote: >> > >> > >> >> > >> > >> > Hi Oleg, >> > >> > >> > >> > >> > >> > I noticed that GridAbstractTest is now capable to run junit4 >> > >> tests. >> > >> > >> > What are the current recommendations for writing new tests? >> Can we >> > >> use >> > >> > >> > junit4 annotation for new tests? >> > >> > >> > пн, 12 нояб. 2018 г. в 19:58, oignatenko < >> > >> > >> > >> > > oignatenko@ >> > >> > >> > >> > > >: >> > >> > >> > > >> > >> > >> > > Hi Ivan, >> > >> > >> > > >> > >> > >> > > I am currently testing approach you used in pull/5354 in >> the >> > >> "pilot" >> > >> > >> > > sub-task with examples tests (IGNITE-10174). >> > >> > >> > > >> > >> > >> > > So far it looks more and more like the way to go. The most >> > >> promising >> > >> > >> > thing I >> > >> > >> > > observed is that after I changed classes in our test >> framework >> > >> the >> > >> > >> way >> > >> > >> > you >> > >> > >> > > did, execution of (unchanged) examples tests went exactly >> the >> > >> same >> > >> > as >> > >> > >> it >> > >> > >> > was >> > >> > >> > > before changes. >> > >> > >> > > >> > >> > >> > > This indicates that existing tests won't be affected making >> it >> > >> > indeed >> > >> > >> low >> > >> > >> > > risk. >> > >> > >> > > >> > >> > >> > > After that I converted examples tests to Junit 4 by adding >> > >> @RunWith >> > >> > >> and >> > >> > >> > > @Test annotations and tried a few, and these looked okay. >> > >> > >> > > >> > >> > >> > > Currently I am running full examples test suite and after >> it is >> > >> over >> > >> > >> I >> > >> > >> > will >> > >> > >> > > compare results to the reference list I made by running it >> prior >> > >> to >> > >> > >> > > migration. >> > >> > >> > > >> > >> > >> > > 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 > > > > -- > Best regards, > Ivan Pavlukhin -- Sent from: http://apache-ignite-developers.2346864.n4.nabble.com/ |
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 <[hidden email]>: > > 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 > > > Павлухин Иван wrote > > Hi guys, > > > > I started writing a junit4 test for a feature I am currently working > > on. And immediately I faced a peculiarity with @Before test fixture. I > > automatically named a method "setUp" and realized that I override a > > method from a superclass. It can produce unwanted effects. First, it > > looks reasonable to call a corresponding superclass method in an > > overridden method, but in my understanding it is not a canonical way > > in junit4. Second, in junit4 overriding method annotated with @Before > > can lead changing order of this method execution among other @Before > > method. Second point might lead to surprising effects in tests > > hierarchies defining @Before methods on different levels. > > > > Let's return to the test I mentioned in the beginning of current > > email. I decided to use a different name for my @Before method > > (actually it was unpleasant "setUp1"). I think that we can do the > > following to make things straightforward: > > 1. Prohibit overriding base class setUp and tearDown methods by making > > them final (at least in GridCommonAbstractTest and > > GridSpiAbstractTest). > > 2. Use common default name for @Before and @After methods defined in > > concrete test classes. I do not have really good candidates for names. > > Simply we can use setUp1, tearDown1. Another option initTest, > > cleanupTest. Your name candidates are quite appreciated. > > > > WDYT? > > > > P.S. Also I went through other tests and found really dangerous thing. > > In IgniteJmsStreamerTest (not migrated to junit4 yet) we have > > beforeTest method (overriding GridAbstractTest framework method) > > annotated with @Before annotation (I believe accidentally)! It means > > that migrating such test to junit4 will lead to calling beforeTest > > twice. Ouch! > > ср, 12 дек. 2018 г. в 13:21, Павлухин Иван < > > > vololo100@ > > > >: > >> > >> Hi Oleg, > >> > >> Thanks your for detailed answer. Using junit4 annotations where > >> possible sounds good to me. > >> > >> Also I would like to thank everybody involved in migration to junit4 > >> process for your efforts. You were able to move a problem which looked > >> mountain-heavy. I believe that is a significant milestone on Ignite > >> stabilization road. > >> > >> Well done! > >> вт, 11 дек. 2018 г. в 20:20, oignatenko < > > > oignatenko@ > > > >: > >> > > >> > Hi Ivan, > >> > > >> > That's a very good question and I think you spotted a point where > >> Ignite > >> > test developer's preferences need to change when migrating from Junit 3 > >> to > >> > 4. > >> > > >> > In brief, when developing under JUnit 3 there were good reasons to > >> prefer > >> > our homebrew template methods (beforeTestsStarted, beforeTest, > >> afterTest, > >> > afterTestsStopped) over those provided by JUnit (setUp / tearDown). To > >> > answer your other question, in order to reliably understand how our > >> methods > >> > work one would better go to (potentially changing) code of > >> GridAbstractTest > >> > and check where and how these are invoked. > >> > > >> > But under JUnit 4 these reasons don't apply anymore and preference > >> should go > >> > the other way, namely in favor of annotations @Before, @After, > >> @BeforeClass, > >> > @AfterClass. > >> > > >> > In other words, when these annotations allow you implement desired test > >> > logic you better use them. These annotations have stable, well > >> documented > >> > and widely understood semantics (and don't require one to skim through > >> 3K > >> > lines of code for grasping details:). > >> > > >> > And it makes sense to "fallback" to our old-fashioned methods only when > >> you > >> > simply have no other choice. For example doing some quick and simple > >> fix in > >> > a legacy code that is filled with multiple usages of our old template > >> > methods it may be okay to choose to keep old style - instead of doing > >> > cumbersome, risky and lengthy refactoring or polluting test code with > >> > inconsistent use of annotations intertwined with old style methods. > >> > > >> > That's what I think. If you're interested in why I think so, refer to > >> boring > >> > explanation in PS. > >> > > >> > regards, Oleg > >> > > >> > ----- > >> > > >> > PS. With regards to old preferences I believe it is very important to > >> keep > >> > in mind that Junit 3 didn't provide means to do initialization and > >> cleanup > >> > once for all test methods in the class. > >> > > >> > So when one needed things to run once per test class they had to go > >> > somewhere else. In Ignite the natural way to go was GridAbstractTest > >> which > >> > provided this functionality via beforeTestsStarted and > >> afterTestsStopped > >> > (with some glitches as I recently learned but still). And after one had > >> to > >> > use these methods there was no compelling reason to use JUnit's setUp > >> and > >> > tearDown, because GridAbstractTest also provided similar methods. > >> > > >> > It was just easier to consistently use full and cohesive set of methods > >> > provided by GridAbstractTest than switch back and forth between it and > >> JUnit > >> > 3 methods. > >> > > >> > With JUnit 4 above reasoning doesn't apply anymore because its features > >> are > >> > just as full and cohesive and can be consistently used without having > >> to > >> > fallback to stuff from GridAbstractTest. So we have a real choice now > >> and > >> > can judge based on other criteria than functional completeness (since > >> both > >> > ways now satisfy that most important criteria). > >> > > >> > And this reveals us a full spectrum of JUnit advantages over > >> > GridAbstractTest (which was obscured before, because JUnit 3 was > >> > functionally incomplete). > >> > > >> > And on that newly opened spectrum JUnit 4 API wins hands down. > >> > > >> > JUnit 4 is much better documented and "googlable", it is stable and > >> much > >> > wider known. The last but not the least, as I already mentioned, it > >> doesn't > >> > require one to skim through 3K lines of code for grasping details and > >> its > >> > semantics doesn't depend on (possibly changing) implementation code. > >> > > >> > Also, initialization and cleanup methods of JUnit 4 make an organic > >> part of > >> > wider, more comprehensive API which smoothly integrates with IDE, > >> Maven, > >> > Teamcity and all imaginable Java tools over there. > >> > > >> > Every time one annotates @Test or @Ignore gives them a reason to prefer > >> > other JUnit annotations over something else. It works about the same as > >> I > >> > described above for beforeTestsStarted in GridAbstractTest and how it > >> > inhibited use of JUnit 3 methods: after you picked one part of some API > >> it > >> > naturally becomes more convenient to keep using other parts of that > >> API. > >> > > >> > Summing up, I believe that every time you have a (sensible) choice > >> between > >> > JUnit 4 and GridAbstractTest methods you just pick JUnit 4 and don't > >> look > >> > back. > >> > > >> > > >> > Павлухин Иван wrote > >> > > Ed, Oleg, > >> > > > >> > > Could you please clarify on the following point: > >> > >> 3. Add @Before, @After on methods which should run before, after a > >> > >> test (setUp, tearDown in current approach). > >> > > Beforehand we used to override beforeTestsStarted, beforeTest, > >> > > afterTest, afterTestsStarted. What will happen with these methods > >> > > after migration to junit4? I can see that order of execution can > >> > > become tricky especially when both styles are used at the same time. > >> > > And an immediate suggestion is to avoid using both styles > >> (annotations > >> > > and overridden methods) in the same test. What is your insight? > >> > > вт, 11 дек. 2018 г. в 10:27, Vladimir Ozerov < > >> > > >> > > vozerov@ > >> > > >> > > >: > >> > >> > >> > >> Hi Oleg, > >> > >> > >> > >> Yes, makes perfect sense. Thank you. > >> > >> > >> > >> On Mon, Dec 10, 2018 at 10:14 PM oignatenko < > >> > > >> > > oignatenko@ > >> > > >> > > > wrote: > >> > >> > >> > >> > Hi Vovan, > >> > >> > > >> > >> > I just created JIRA ticket to address your concerns: > >> > >> > - https://issues.apache.org/jira/browse/IGNITE-10629 > >> > >> > > >> > >> > In brief, the plan is that a week or two after migration is over > >> we > >> > >> will > >> > >> > run > >> > >> > code inspection that detects JUnit 3 style tests that lack @Test > >> > >> annotation > >> > >> > and fix these tests if there are any. > >> > >> > > >> > >> > Does that answer your question? > >> > >> > > >> > >> > regards, Oleg > >> > >> > Vladimir Ozerov wrote > >> > >> > > Ed, > >> > >> > > > >> > >> > > Several questions from my side: > >> > >> > > 1) When the change is expected to be merged? > >> > >> > > 2) What contributors with opened PRs and new or updated JUnit3 > >> tests > >> > >> are > >> > >> > > supposed to do? Rewrite to JUnit4? > >> > >> > > > >> > >> > > If yes, then we should give them time to have a chance to get > >> used to > >> > >> new > >> > >> > > approach and resolve possible conflicts. > >> > >> > > > >> > >> > > Vladimir. > >> > >> > > > >> > >> > > пн, 10 дек. 2018 г. в 20:32, Eduard Shangareev < > >> > >> > > >> > >> > > eduard.shangareev@ > >> > >> > > >> > >> > > >: > >> > >> > > > >> > >> > >> Ivan, > >> > >> > >> > >> > >> > >> So, suggested actions with the new approach: > >> > >> > >> 1. Add @Test annotation on test methods. > >> > >> > >> 2. Add @RunWith(JUnit4.class) annotation on test class. > >> > >> > >> 3. Add @Before, @After on methods which should run before, > >> after a > >> > >> > >> test (setUp, tearDown in current approach). > >> > >> > >> 4. Add your test-class to a suite using suite.addTest(new > >> > >> > >> JUnit4TestAdapter(YourTestClass.class)); > >> > >> > >> 5. Use @Ignore instead fail(); for muting test. > >> > >> > >> 6. You could start using @Parametrized instead of inheritance. > >> > >> > >> > >> > >> > >> > >> > >> > >> On Mon, Dec 3, 2018 at 1:15 PM Павлухин Иван < > >> > >> > > >> > >> > > vololo100@ > >> > >> > > >> > >> > > > wrote: > >> > >> > >> > >> > >> > >> > Hi Oleg, > >> > >> > >> > > >> > >> > >> > I noticed that GridAbstractTest is now capable to run junit4 > >> > >> tests. > >> > >> > >> > What are the current recommendations for writing new tests? > >> Can we > >> > >> use > >> > >> > >> > junit4 annotation for new tests? > >> > >> > >> > пн, 12 нояб. 2018 г. в 19:58, oignatenko < > >> > >> > > >> > >> > > oignatenko@ > >> > >> > > >> > >> > > >: > >> > >> > >> > > > >> > >> > >> > > Hi Ivan, > >> > >> > >> > > > >> > >> > >> > > I am currently testing approach you used in pull/5354 in > >> the > >> > >> "pilot" > >> > >> > >> > > sub-task with examples tests (IGNITE-10174). > >> > >> > >> > > > >> > >> > >> > > So far it looks more and more like the way to go. The most > >> > >> promising > >> > >> > >> > thing I > >> > >> > >> > > observed is that after I changed classes in our test > >> framework > >> > >> the > >> > >> > >> way > >> > >> > >> > you > >> > >> > >> > > did, execution of (unchanged) examples tests went exactly > >> the > >> > >> same > >> > >> > as > >> > >> > >> it > >> > >> > >> > was > >> > >> > >> > > before changes. > >> > >> > >> > > > >> > >> > >> > > This indicates that existing tests won't be affected making > >> it > >> > >> > indeed > >> > >> > >> low > >> > >> > >> > > risk. > >> > >> > >> > > > >> > >> > >> > > After that I converted examples tests to Junit 4 by adding > >> > >> @RunWith > >> > >> > >> and > >> > >> > >> > > @Test annotations and tried a few, and these looked okay. > >> > >> > >> > > > >> > >> > >> > > Currently I am running full examples test suite and after > >> it is > >> > >> over > >> > >> > >> I > >> > >> > >> > will > >> > >> > >> > > compare results to the reference list I made by running it > >> prior > >> > >> to > >> > >> > >> > > migration. > >> > >> > >> > > > >> > >> > >> > > 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 > > > > > > > > -- > > Best regards, > > Ivan Pavlukhin > > > > > > -- > Sent from: http://apache-ignite-developers.2346864.n4.nabble.com/ -- Best regards, Ivan Pavlukhin |
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/ |
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 <[hidden email]>: > > 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 |
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/ |
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 <[hidden email]>: > > 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 |
Hi Igniters,
I've tried yesterday to find any of @Ignore d tests with a link to the issue in its value on TeamCity, but I did not manage to. Do you know if JUnit Ignored tests are completely invisible in the TC and its REST? I've also tried to google it, but not found anything yet. I remember that .NET Ignores are at least visible in the UI and in a REST request (see, for example, https://ci.ignite.apache.org/viewLog.html?buildId=2590524&tab=buildResultsDiv&buildTypeId=IgniteTests24Java8_PlatformNet&branch_IgniteTests24Java8=%3Cdefault%3E ). Please advice me or share a link to a build with Java JUnit Ignores, which is visible in TC. Sincerely, Dmitriy Pavlov ср, 19 дек. 2018 г. в 11:59, Павлухин Иван <[hidden email]>: > 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 <[hidden email]>: > > > > 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 > |
In reply to this post by Ivan Pavlukhin
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/ |
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 <[hidden email]>: > > 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 |
In reply to this post by Vladimir Ozerov
...sort of a follow-up. I gave folks a bit more leeway than originally
planned week or two. It's been about a month since master was changed in a way that wouldn't allow JUnit 3 tests execute anymore (per IGNITE-10177) and I just started progress on IGNITE-10629 and made a promised inspection for tests that could slip through without required annotation while we were migrating the project to JUnit 4. Results of this check look pretty good, IDEA reported only 6 (six) troublesome test cases (4 of which were by the way annotated @Ignore, meaning that even with added @Test these won't run anyway). Compared to over 11,600 properly annotated test cases this looks really minor. We probably can say that tests in master migrated to JUnit 4 quite smoothly. regards, Oleg Vladimir Ozerov wrote > Hi Oleg, > > Yes, makes perfect sense. Thank you. > > On Mon, Dec 10, 2018 at 10:14 PM oignatenko < > oignatenko@ > > wrote: > >> Hi Vovan, >> >> I just created JIRA ticket to address your concerns: >> - https://issues.apache.org/jira/browse/IGNITE-10629 >> >> In brief, the plan is that a week or two after migration is over we will >> run >> code inspection that detects JUnit 3 style tests that lack @Test >> annotation >> and fix these tests if there are any. >> >> Does that answer your question? >> >> regards, Oleg >> Vladimir Ozerov wrote >> > Ed, >> > >> > Several questions from my side: >> > 1) When the change is expected to be merged? >> > 2) What contributors with opened PRs and new or updated JUnit3 tests >> are >> > supposed to do? Rewrite to JUnit4? >> > >> > If yes, then we should give them time to have a chance to get used to >> new >> > approach and resolve possible conflicts. >> > >> > Vladimir. >> > >> [cut] >> >> -- >> Sent from: http://apache-ignite-developers.2346864.n4.nabble.com/ >> -- Sent from: http://apache-ignite-developers.2346864.n4.nabble.com/ |
Cheers!
вт, 22 янв. 2019 г. в 20:19, oignatenko <[hidden email]>: > > ...sort of a follow-up. I gave folks a bit more leeway than originally > planned week or two. > > It's been about a month since master was changed in a way that wouldn't > allow JUnit 3 tests execute anymore (per IGNITE-10177) and I just started > progress on IGNITE-10629 and made a promised inspection for tests that could > slip through without required annotation while we were migrating the project > to JUnit 4. > > Results of this check look pretty good, IDEA reported only 6 (six) > troublesome test cases (4 of which were by the way annotated @Ignore, > meaning that even with added @Test these won't run anyway). Compared to over > 11,600 properly annotated test cases this looks really minor. We probably > can say that tests in master migrated to JUnit 4 quite smoothly. > > regards, Oleg > > > Vladimir Ozerov wrote > > Hi Oleg, > > > > Yes, makes perfect sense. Thank you. > > > > On Mon, Dec 10, 2018 at 10:14 PM oignatenko < > > > oignatenko@ > > > > wrote: > > > >> Hi Vovan, > >> > >> I just created JIRA ticket to address your concerns: > >> - https://issues.apache.org/jira/browse/IGNITE-10629 > >> > >> In brief, the plan is that a week or two after migration is over we will > >> run > >> code inspection that detects JUnit 3 style tests that lack @Test > >> annotation > >> and fix these tests if there are any. > >> > >> Does that answer your question? > >> > >> regards, Oleg > >> Vladimir Ozerov wrote > >> > Ed, > >> > > >> > Several questions from my side: > >> > 1) When the change is expected to be merged? > >> > 2) What contributors with opened PRs and new or updated JUnit3 tests > >> are > >> > supposed to do? Rewrite to JUnit4? > >> > > >> > If yes, then we should give them time to have a chance to get used to > >> new > >> > approach and resolve possible conflicts. > >> > > >> > Vladimir. > >> > > >> [cut] > >> > >> -- > >> Sent from: http://apache-ignite-developers.2346864.n4.nabble.com/ > >> > > > > > > -- > Sent from: http://apache-ignite-developers.2346864.n4.nabble.com/ -- Best regards, Ivan Pavlukhin |
Free forum by Nabble | Edit this page |