Page MenuHomePhabricator

tests: Major code cleanup
ClosedPublic

Authored by vtorri on Feb 4 2016, 6:19 AM.

Details

Summary

Factorize the test infra.
Uniformize all the tests (function name convention)

The following test components have been refactored:

  • Ecore, Ecore_Cxx, Ector
  • Ecore_Con (maybe should be moved inside ecore test)
  • Edje
  • Eet (split into different files)
  • Eet_Cxx
  • Eeze (also split)
  • Efreet (needs more love!)
  • Eina (minor changes: whitespaces, order of includes, ...)
  • Eina_Cxx
  • Eio
  • ElDbus, ElDbus_Cxx
  • Elua
  • Emile
  • Eo (under suite/ only)
  • Eolian, Eolian_Cxx
  • Evas
  • Evil

So far unmodified:

  • elocation
  • efl_js, eolian_js
  • emotion

Thanks to this, setenv("CK_FORK", "no", 0); should become useless.

This patch should remove about 2K lines of duplicated code.

Test Plan

make check

Diff Detail

Repository
rEFL core/efl
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
vtorri updated this revision to Diff 8354.Feb 4 2016, 6:19 AM
vtorri retitled this revision from to Test rework.
vtorri updated this object.
vtorri edited the test plan for this revision. (Show Details)
vtorri added reviewers: cedric, jpeg, tasn.
vtorri updated this revision to Diff 8355.Feb 4 2016, 6:28 AM
  • Add efl_check.h to eina unit test files as Eina is mandatory
vtorri added a comment.Feb 4 2016, 6:34 AM

Some comments :

  1. i've not touched :
    • elocation (this one is easy to do, 'ill do it later if I have some time)
    • efl_js, emotion and eolian_js : I don't know what to do
    • ecore_audio_cxx, edje_cxx and evas_cxx : the unit tests are just to see if the headers are compilable
  1. ecore_con shoul go into ecore unit tests, like ecore_x, ecore_evas, etc...
  1. setenv("CK_FORK", "no", 0); should be useless, now
  1. evas_suite.c : evas_init() has no reason to be here, but i've let it there
  1. evas_test_render_engines.c : the list of the engines should be got from the evas API, no ?
  1. eolian : eolian_parsing.c : should be renamed, I think and one should look inside deeper to see if there are some imprvements to do
  1. I have added efl_check.h to Makefile_Eina.am (in the unit test files) as I don't really know if there is a better solution

feel free to comment

vtorri added a comment.Feb 4 2016, 6:49 AM

also, the stats are 199 files changed, 4769 insertions(+), 5867 deletions(-)

so 1098 line less

tasn requested changes to this revision.Feb 4 2016, 8:01 AM
tasn edited edge metadata.

Hey dude, thanks for the patch, but it's impossible to understand what's going on.

First of all, could you please explain in the commit message what you actually did? "Text rework" means nothing.
Also, could you maybe start with a patch to one suite, e.g. eina, so we can review and see the changes themselves without all the extra changes for all the other suites?

Essentially, I wanna understand what's going on, and I want the commit message to reflect that too.

Thanks!

This revision now requires changes to proceed.Feb 4 2016, 8:01 AM
vtorri added a comment.Feb 4 2016, 8:24 AM

Basically, there are a lot of duplication code. See efl_check.h and evil_suite.c to see the difference ("Factorisation of the infra"). That is the main point of the patch.
The rest is only formatting, reorganising headers and function names (" make uniform all the tests") so that they follow the same scheme. I decided to do all that at the same time because I will not have free time now (for personal reasons).

I will be able to fix some stuf here and there, but i won't have time to redo all this work.

if you don't plan to apply it, then close the bug
i won't do any change, no time

tasn added a comment.Feb 10 2016, 3:54 AM

I'll keep it open for now, and maybe i'll get to fixing it up myself. As it is, it can't go in. Commit message is not conforming to our guidelines, and it'll be too hard to review for others reviewing, or for me.

jpeg edited edge metadata.Feb 15 2016, 8:34 PM

I believe @vtorri has done a good job here, and we should finish polishing up this patch and merge it, since he won't have enough time to spend on it in the near future.

jpeg retitled this revision from Test rework to tests: Major code cleanup.Feb 15 2016, 8:44 PM
jpeg updated this object.
jpeg edited edge metadata.
jpeg added a project: efl.
tasn added a comment.Feb 16 2016, 2:29 AM

I agree, and since the new phab update, I can see the change is already split to commits like I asked!

@vtorri, could you please send us the separate commits somewhere? I'm sorry, it's just that phab is shit with multicommit patches.

tasn added a comment.Feb 16 2016, 4:47 AM

This is too big to review here, so we are reviewing externally. Keeping this open for tracking.

Closed by commit rEFL66b60da444ad: Test rework (authored by vtorri, committed by tasn). · Explain WhyFeb 16 2016, 5:11 AM
This revision was automatically updated to reflect the committed changes.