Unittest for wayland ivi extention #180
Conversation
This folder consists of 3 parts: - client: test ivi-layermanagement-api (ilmCommon, ilmControl, ilmInput). - server: test ivi-id-agent-modules, ivi-input-modules and weston-ivi-shell. - common: contains common fake APIs for both client and server. Signed-off-by: Au Doan Ngoc <[email protected]>
Add the unittest folder to cmake file. Signed-off-by: Au Doan Ngoc <[email protected]>
To build with unittests, need to use flag BUILD_ILM_UNIT_TESTS. Signed-off-by: Au Doan Ngoc <[email protected]>
| ASSERT_EQ(ILM_SUCCESS, ilm_surfaceSetVisibility(l_surfaceId, l_newVisibility)); | ||
|
|
||
| ASSERT_EQ(1, wl_proxy_marshal_flags_fake.call_count); | ||
| ASSERT_EQ(1, wl_display_flush_fake.call_count); |
There was a problem hiding this comment.
Should we check for arguments passed to wl_proxy_marshal_flags_fake (like if we this call was made with the right opcode[IVI_WM_SET_SURFACE_VISIBILITY] and correct surface id & visibility)
ASSERT_EQ(IVI_WM_SET_SURFACE_VISIBILITY, wl_proxy_marshal_flags_fake.arg1_history[0]);
ASSERT_EQ(l_surfaceId, wl_proxy_marshal_flags_fake.arg5_history[0]);
ASSERT_EQ(newVisibility, wl_proxy_marshal_flags_fake.arg6_history[0]);
There was a problem hiding this comment.
Also if version information can be checked?
There was a problem hiding this comment.
Above should be taken care in all test case where we have asserted success and we expect wl_proxy_marshal_flags_fake() to be called.
There was a problem hiding this comment.
Applied the checking for the second argument for all tests are calling wl_proxy_marshal_flags_fake. wl_proxy_marshal_flags is a Variadic function, so we only check for first five arguments (0->4). so it means, we can't check the data of request.
There was a problem hiding this comment.
ohh, yes!!
But I found something here: meekrosoft/fff#57
Might be we can use custom fake function?
There was a problem hiding this comment.
Or atleast
ASSERT_EQ(IVI_WM_SET_SURFACE_VISIBILITY, wl_proxy_marshal_flags_fake.arg1_history[0]);
can be verified.
There was a problem hiding this comment.
Or atleast
ASSERT_EQ(IVI_WM_SET_SURFACE_VISIBILITY, wl_proxy_marshal_flags_fake.arg1_history[0]);can be verified.
This thing is done.
There was a problem hiding this comment.
ohh, yes!! But I found something here: meekrosoft/fff#57 Might be we can use custom fake function?
I need to look more. thanks for the link.
|
|
||
| void init_controller_content() | ||
| { | ||
| uint8_t l_fakeResource = 0, l_fakeClient = 0; |
There was a problem hiding this comment.
refactor this function to more simple:
- Fix a scene graph, define and define it more detail here.
- consider to add new internal functions for: add/remove/get_surface, add/remove/get_layer, add/remove/get_screen, add/remove/get_surface_properties, add/remove/get_surface_notification, add/remove/get_layer_properties, add/remove/get_layer_notfication ... TOO MUCH :(
There was a problem hiding this comment.
we will consider to do this thing in refactor action in the future
|
we should create a parent folder |
| * -# Mocking the surface_get_weston_surface() does return an object | ||
| * -# Calling the surface_event_create() | ||
| * -# Verification point: | ||
| * +# get_id_of_surface() must be called once time |
There was a problem hiding this comment.
I think there are some redundant verification points:
The behaviors of the list (e.g. init, insert, ...) are not required to be checked in this test case (also I see some test cases do the same thing). These behaviors we can separate to one concrete test case for the list.
The only case that needs to be checked is when the behaviors are the result of the sequence in the test case.
| * +# wl_proxy_marshal_flags() must be called once time | ||
| * +# wl_display_roundtrip_queue() must be called once time | ||
| */ | ||
| TEST_F(IlmControlTest, ilm_surfaceGetOpacity_invalidLayerId) |
There was a problem hiding this comment.
Here I think we are checking for invalid surface id name and description should be accordingly
| EXPECT_EQ(get_id_of_surface_fake.call_count, 1); | ||
| EXPECT_EQ(get_properties_of_surface_fake.call_count, 1); | ||
| EXPECT_EQ(surface_get_weston_surface_fake.call_count, 1); |
There was a problem hiding this comment.
These verification points are redundant in my opinion, because in the surface_event_create_idDifferentBkgnSurfaceId, we have already verified these points for surface_event_create() function, there is no need to verify it again. The redundant verification points for a unit (e.g. function) make more effort in the future when we modify the code, then we need to modify all test cases to verify the same point.
There was a problem hiding this comment.
From my opinion, we should keep to check in each testcase, because if we just verify these condition in case surface_event_create_idDifferentBkgnSurfaceId, and when we only want to run case surface_event_create_idSameBkgnSurfaceId (for example run with gtest flag --gtest_filter), these function will not be verifed
| int ControllerTests::ms_idAgentModuleValue = 0; | ||
| int ControllerTests::ms_inputControllerModuleValue = 0; | ||
| uint32_t ControllerTests::ms_screenIdOffset = 0; | ||
| uint32_t ControllerTests::ms_screenId = 0; | ||
| char *ControllerTests::ms_iviClientName = nullptr; | ||
| char *ControllerTests::ms_debugScopes = nullptr; | ||
| char *ControllerTests::ms_screenName = nullptr; | ||
| int32_t ControllerTests::ms_bkgndSurfaceId = 0; | ||
| uint32_t ControllerTests::ms_bkgndColor = 0; | ||
| bool ControllerTests::ms_enableCursor = false; | ||
| uint32_t ControllerTests::ms_westonConfigNextSection = 0; |
There was a problem hiding this comment.
This can be done in ControlerContext class, which would be separated from ControlerTest classe
There was a problem hiding this comment.
we will consider to do this thing in refactor action in the future
| g_SurfaceCreatedCount++; | ||
| } | ||
|
|
||
| class ControllerTests : public ::testing::Test |
There was a problem hiding this comment.
I think this class (e.g. ControlerTests) should only aim for SetUp() and TearDown(). The controller content should be a separate class.
There was a problem hiding this comment.
we will consider to do this thing in refactor action in the future
chienpv1
force-pushed
the
unittest_for_wayland_ivi_extention
branch
from
f1cef61 to
db3946a
Compare
These unittest are written for weston 9. Now weston version is 12 In this commit, let disable some testcase called removed APIs, delete get function pointer of private function in source code, let include directly source file and call private function directly. Signed-off-by: Phung Van Chien <[email protected]>
Screenshot feature have new public APIs. Some exist APIs are changed with new logic and mechanism Create unittest for new APIs, update out-of-date testcase. Signed-off-by: Phung Van Chien <[email protected]>
Signed-off-by: Tran Ba Khang(MS/EMC31-XC) <[email protected]>
Signed-off-by: Tran Ba Khang(MS/EMC31-XC) <[email protected]>
Update unittest in file ivi_controller_uinttests.cpp: - Create new testcases for new APIs - Changes method free resource when testsuite is running - Add more necessary verification point for exist testcases - Clear code test Signed-off-by: Phung Van Chien <[email protected]>
Signed-off-by: Tran Ba Khang(MS/EMC31-XC) <[email protected]>
Signed-off-by: Tran Ba Khang(MS/EMC31-XC) <[email protected]>
Update unittest in file ilm_control_unittests.cpp, fix cmake warning:
- Change minimum required version of cmake when build unittests
- Create new testcases for new APIs
- Changes method free resource when testsuite is running
- Add more necessary verification point for exist testcases
- Clear code test
Signed-off-by: Phung Van Chien <[email protected]>
Signed-off-by: Tran Ba Khang(MS/EMC31-XC) <[email protected]>
Add new testcases in file ivi_controller_uinttests.cpp, edit cmake
option when build unittest:
- Create 2 new testcases to testing for 2 cases:
+ surface_event_create() is called when a desktop surface is created
+ output_created_event() is called when ivi shell have
background view and client are available
- Change cmake option when build unittest to better name
Signed-off-by: Phung Van Chien <[email protected]>
chienpv1
force-pushed
the
unittest_for_wayland_ivi_extention
branch
from
db3946a to
e1e348a
Compare
|
tried to build on top of the current master and observed many warning and also build error! please check the attachment gtest version 1.10.0-2 |
could you please recheck the weston version. because these thing is adapt to build and run with weston 12. and weston 12 require version of wayland >= 1.20.0 |
This PR was created to replace PR_150 for more convenient reviews and updates