ignite-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From oignatenko <oignate...@gridgain.com>
Subject Re: Is it time to move forward to JUnit4 (5)?
Date Sun, 16 Dec 2018 14:32:33 GMT
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, Павлухин Иван &lt;

> vololo100@

> &gt;:
>>
>> 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 &lt;

> oignatenko@

> &gt;:
>> >
>> > 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 &lt;
>> >
>> > > vozerov@
>> >
>> > > &gt;:
>> > >>
>> > >> Hi Oleg,
>> > >>
>> > >> Yes, makes perfect sense. Thank you.
>> > >>
>> > >> On Mon, Dec 10, 2018 at 10:14 PM oignatenko &lt;
>> >
>> > > oignatenko@
>> >
>> > > &gt; 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 &lt;
>> > >> >
>> > >> > > eduard.shangareev@
>> > >> >
>> > >> > > &gt;:
>> > >> > >
>> > >> > >> 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 Павлухин Иван
&lt;
>> > >> >
>> > >> > > vololo100@
>> > >> >
>> > >> > > &gt; 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
&lt;
>> > >> >
>> > >> > > oignatenko@
>> > >> >
>> > >> > > &gt;:
>> > >> > >> > >
>> > >> > >> > > 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/

Mime
View raw message