ignite-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Павлухин Иван <vololo...@gmail.com>
Subject Re: Is it time to move forward to JUnit4 (5)?
Date Sun, 11 Nov 2018 14:22:48 GMT
Hi,

I have rerun RunAll for the aforementioned PR [1]. Results [2] look
very similar to ones which we currently have for master.

[1] https://github.com/apache/ignite/pull/5354/files
[2] https://ci.ignite.apache.org/viewLog.html?buildId=2282588&tab=buildResultsDiv&buildTypeId=IgniteTests24Java8_RunAll
пт, 9 нояб. 2018 г. в 17:30, Павлухин Иван <vololo100@gmail.com>:
>
> Hi Oleg,
>
> I can outline some motivating ideas. There is no agreed solutions yet,
> but junit4 might help us in reusing existing tests for improving MVCC
> coverage. Another area is managing RunAll execution time with help
> of some kind of tests parameterization.
>
> Regarding concrete tests changes from my experiment [1]. They were
> changed in order to demonstrate certain capabilities in action.
> 1. CacheMvccDmlSimpleTest simply demonstrates that it is a valid
> junit4 test. Test names without "test" prefix make it evident that tests
> are run by junit4 engine.
> 2. IgniteCacheCommitDelayTxRecoveryTest shows how a pattern
> common for our junit3 tests could be improved by junit4.
>
> I did not have each test method renaming in mind (but I suppose we
> cannot avoid marking each test with @Test annotation).
>
> And I also have run RunAll after introducing changes in GridAbstractTest [2].
> There were several rebuilds because of problems found along the way. I will
> return to it in order to receive good enough single RunAll result eligible for
> analysis by human.
>
> [1] https://github.com/apache/ignite/pull/5354/files
> [2] https://mtcga.gridgain.com/pr.html?serverId=apache&suiteId=IgniteTests24Java8_RunAll&branchForTc=pull/5354/head&action=Latest
>
>
> пт, 9 нояб. 2018 г. в 16:48, oignatenko <oignatenko@gridgain.com>:
>>
>> Hi Ivan,
>>
>> That's a very good question. I would say the primary motivation in the task
>> we discuss is to make the change at the minimal cost and risk. Or, as stated
>> in description of IGNITE-10173 "move... gradually, smoothly and safely".
>>
>> With this in mind I would say that main expected benefit is laid out at the
>> top of ticket description:
>> > Currently unit tests in the project are mix of two versions Junit 3 and 4.
>> > This makes it hard to develop and maintain. Making all tests use the same
>> > version Junit is intended to address this problem.
>>
>> Above goal is probably modest but given that this change is expected to take
>> minimal effort it looks good enough for me.
>>
>> On a more general note I don't expect strong technical benefits of this
>> change, at least not immediately. This is based on my studies of prior
>> discussions related to this matter and of the existing tests. I believe that
>> if there were any strong and obvious technical benefits we would be long
>> aware of these and we would migrate to newer Junit long time ago.
>>
>> Observing that migration hasn't happened - despite JUnit 3 being obsolete
>> for over 10 years now, and despite many more fundamental changes that
>> successfully made it to Ignite in past years - it looks natural to assume
>> that there are just no technical reasons that would be strong enough to
>> justify investing much effort in the migration.
>>
>> ---
>>
>> Now about your experiment at ignite/pull/5354, I briefly skimmed it and here
>> are some initial impressions.
>>
>> First the bad news, some parts look indeed "insane" to me. I am talking
>> specifically about renames in CacheMvccDmlSimpleTest and changes to test
>> design in IgniteCacheCommitDelayTxRecoveryTest. I believe that these are
>> wildly out of scope of the discussed change because I firmly want to keep it
>> limited to absolute minimum of changes (ideally only annotations with
>> involved imports).
>>
>> This is because there are just too many tests to (manually) change. My rough
>> estimate is there about 7,000 (seven thousands!) test cases to change in
>> core module alone; and I guess there are few thousands more in other parts
>> of the project. Doing any extra changes to these tests makes things much
>> more complicated, effort consuming and risky than was initially supposed.
>>
>> On a brighter side, changes made to GridAbstractTest look worth further
>> studying. At this point I still prefer to keep this class unchanged (because
>> this gives us a bulletproof guarantee against regressions in all prior
>> tests) but this may change after we do "pilot" changes to examples tests
>> (IGNITE-10174).
>>
>> These pilot changes will help us estimate with concrete code how much effort
>> could be involved in maintaining "twin" classes in sync (you are absolutely
>> right pointing that this is concerning). If it turns out too complicated to
>> sync we would probably switch to something like you have done in pull/5354
>>
>> regards, Oleg
>>
>>
>> Павлухин Иван wrote
>> > Hi Oleg,
>> >
>> > Migrating to Junit 4 sounds as great idea. I believe everybody
>> > is quite surprised when finds Junit 3 in Ignite.
>> > But for me personally it is good to understand what problem we
>> > are trying to solve? What benefits will Junit 4 give us? What are
>> > the most painful moments with current testing framework?
>> > (I my mind I draw a proposal which contains sections "Motivation",
>> > "Alternatives", etc.)
>> >
>> > Also I must say that I made some experiments related to Junit4
>> > migration. And from the first glance it seems that the most
>> > important Ignite test framework classes contain a huge amount of
>> > utility methods and a little amount of code related to a test lifecycle.
>> > These classes are GridAbstractTest and GridCommonAbstractTest.
>> > Both are quite thick, but as already said it is mostly utility code
>> > which could be extracted. I would be great to come to very thin base
>> > test classes (or even to baseclass-less tests at all) and include all
>> > kind of utilities without inheritance.
>> >
>> > But massive refactoring perhaps is more for future. The main point
>> > here is that Ignite test framework is not complex at all and could be
>> > easily adopted to Junit 4.
>> >
>> > Regarding twin classes. How to keep them in sync? If the migration
>> > process will be paused halfway it could make things complicated in
>> > future.
>> >
>> > Additionally, I have a wild idea how to marry junit 3 and 4 without
>> > introducing twins. The idea looks ugly from OOP perspective, but
>> > here it is. We can simply annotate overridden Junit 3 setUp and
>> > tearDown methods with @Before and @After annotations. And
>> > use runTest overridden method for Junit 3 and TestRule for Junit 4
>> > in order to run test methods in newly started thread.
>> >
>> > For all details you can see an experiment on github [1]. Among them
>> > are ticks with @RunWith annotation making possible to run a test
>> > inherited from TestCase as Junit 4 test. Still, I might have missed
>> > something.
>> >
>> > Does it sound sane anyhow?
>> >
>> > [1] https://github.com/apache/ignite/pull/5354/files
>> >
>> >
>> > ср, 7 нояб. 2018 г. в 19:57, oignatenko &lt;
>>
>> > oignatenko@
>>
>> > &gt;:
>> >
>> >> Hello,
>> >>
>> >> I created JIRA task for this move, with detailed explanation and
>> >> step-by-step subtasks, your comments are welcome:
>> >>
>> >> - https://issues.apache.org/jira/browse/IGNITE-10173
>> >>
>> >> In brief, the plan is to gradually migrate the part of tests that still
>> >> uses
>> >> Junit 3 (hundreds if not thousands of those that depend on
>> >> GridAbstractTest
>> >> and its subclasses) to newer version. The trick proposed to avoid doing
>> >> all
>> >> this in one (big and risky) step is to introduce a Junit4-based "twin" of
>> >> GridAbstractTest (and respective twins of its subclasses) and use it to
>> >> gradually change tests to use that newer API instead.
>> >>
>> >> After above is over, Junit 3 and all its related stuff (including
>> >> GridAbstractTest) could be safely removed from project.
>> >>
>> >> 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



-- 
Best regards,
Ivan Pavlukhin

Mime
View raw message