Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

test(switch): Initial unit tests #2794

Merged
merged 13 commits into from
Dec 7, 2021
Merged

test(switch): Initial unit tests #2794

merged 13 commits into from
Dec 7, 2021

Conversation

C47D
Copy link
Contributor

@C47D C47D commented Nov 11, 2021

Description of the feature or fix

A clear and concise description of what the bug or new feature is.

Checkpoints

@C47D C47D changed the title test(switch): Add test file and state at creation test test(switch): Unitial unit tests Nov 11, 2021
@kisvegabor kisvegabor changed the title test(switch): Unitial unit tests test(switch): Initial unit tests Nov 11, 2021
@C47D
Copy link
Contributor Author

C47D commented Nov 11, 2021

@kisvegabor @embeddedt Hi,
If you have suggestions for more tests please let me know.

@kisvegabor
Copy link
Member

kisvegabor commented Nov 12, 2021

I have a few ideas:

  • The calculation in LV_EVENT_REFR_EXT_DRAW_SIZE
  • Draw tests to be sure style properties are applied correctly (mainly paddings)
  • Create/delete a bunch of switches in a loop and test for memory leak
  • Test the animation by stepping tick manually and guess where should the animation be.

@embeddedt
Copy link
Member

We could also add a test for #2785.

@C47D
Copy link
Contributor Author

C47D commented Nov 16, 2021

NOTE: lv_event_get_original_target is mentioned in the latest docs, but it's not in the API. https://docs.lvgl.io/master/overview/event.html#fields-of-lv-event-t


We could also add a test for #2785.

From what I understand, we should test that when a switch has event bubbling enabled, the state should change when it's clicked on, is that right? Does 2ca2b0f looks ok?

@C47D
Copy link
Contributor Author

C47D commented Nov 16, 2021

Test the animation by stepping tick manually and guess where should the animation be.

Do you mean try to guess where the switch indicator should be?

@kisvegabor
Copy link
Member

NOTE: lv_event_get_original_target is mentioned in the latest docs, but it's not in the API. https://docs.lvgl.io/master/overview/event.html#fields-of-lv-event-t

Thanks, fixed here: cdd5128

From what I understand, we should test that when a switch has event bubbling enabled, the state should change when it's clicked on, is that right? Does 2ca2b0f looks ok?

Would it be simpler to check whether the switch is checked after the click? (With lv_obj_has_state(sw, LV_STATE_CHECKED)).

Do you mean try to guess where the switch indicator should be?

Yes, with some tolerance on start, mid and end positions. The position of the knob is not known because it's calculated on the drawing but sw->anim_state can be used.

@C47D
Copy link
Contributor Author

C47D commented Nov 18, 2021

The calculation in LV_EVENT_REFR_EXT_DRAW_SIZE

Do you mean this piece of code?

if(code == LV_EVENT_REFR_EXT_DRAW_SIZE) {
lv_coord_t knob_left = lv_obj_get_style_pad_left(obj, LV_PART_KNOB);
lv_coord_t knob_right = lv_obj_get_style_pad_right(obj, LV_PART_KNOB);
lv_coord_t knob_top = lv_obj_get_style_pad_top(obj, LV_PART_KNOB);
lv_coord_t knob_bottom = lv_obj_get_style_pad_bottom(obj, LV_PART_KNOB);
/*The smaller size is the knob diameter*/
lv_coord_t knob_size = LV_MAX4(knob_left, knob_right, knob_bottom, knob_top);
knob_size += 2; /*For rounding error*/
knob_size += lv_obj_calculate_ext_draw_size(obj, LV_PART_KNOB);
lv_coord_t * s = lv_event_get_param(e);
*s = LV_MAX(*s, knob_size);
*s = LV_MAX(*s, lv_obj_calculate_ext_draw_size(obj, LV_PART_INDICATOR));
}

Draw tests to be sure style properties are applied correctly (mainly paddings)

Shouldn't this be tested somewhere else and not in the switch specific tests?

Nevermind, I think I understand switch animation now.

This is the first draft of the passing test, 50 is arbitrary, i just wanted time to pass so the anim_state would be incremented in lv_switch_anim_exec_cb

void test_switch_animation(void)
{
    uint32_t target_time = 0;
    uint32_t time = 0;
    lv_switch_t * anim_sw = (lv_switch_t *) sw;
    int32_t initial_anim_state = anim_sw->anim_state;

    /* Trigger animation */
    lv_test_mouse_click_at(sw->coords.x1, sw->coords.y1);

    time = custom_tick_get();
    target_time = time + 50;

    while (time < target_time) {
        time = custom_tick_get();
    }

    TEST_ASSERT_GREATER_THAN(initial_anim_state, anim_sw->anim_state);
}

@C47D
Copy link
Contributor Author

C47D commented Nov 19, 2021

Create/delete a bunch of switches in a loop and test for memory leak

Here's the test, but it's failing, am I missing something?

#define SWITCHES_CNT    10

void test_switch_should_not_leak_memory_after_deletion(void)
{
    size_t idx = 0;
    uint32_t initial_available_memory = 0;
    uint32_t final_available_memory = 0;
    lv_mem_monitor_t monitor;
    lv_obj_t *switches[SWITCHES_CNT] = {NULL};

    for (idx = 0; idx < SWITCHES_CNT; idx++) {
        switches[idx] = lv_switch_create(scr);
    }

    lv_mem_monitor(&monitor);
    initial_available_memory = monitor.free_size;

    for (idx = 0; idx < SWITCHES_CNT; idx++) {
        lv_obj_del(switches[idx]);
    }

    lv_mem_monitor(&monitor);
    final_available_memory = monitor.free_size;

    TEST_ASSERT_EQUAL_UINT32(final_available_memory, initial_available_memory);
}

This is the output:

./src/test_cases/test_switch.c:65:test_switch_should_not_leak_memory_after_deletion:FAIL: Expected 2084608 Was 2081376

@kisvegabor
Copy link
Member

 lv_mem_monitor(&monitor);
 initial_available_memory = monitor.free_size;

should come before creating the switches to measure the memory without switches.

@C47D
Copy link
Contributor Author

C47D commented Nov 23, 2021

@kisvegabor Sorry I just had some free time to work on the remaining test. Is the animation test similar to what you did expect? I think the knob went from left to right, if we click the switch again, the knob would be on the left again, am I right?

@C47D
Copy link
Contributor Author

C47D commented Nov 24, 2021

I have a few ideas:

* The calculation in `LV_EVENT_REFR_EXT_DRAW_SIZE`

Where is the extended size stored? I've placed a break point in that branch of the lv_switch_event function, but didn't knew where the lv_coord_t *s variable is pointing at.

EDIT:
Is it stored in ext_draw_size of the _lv_obj_spec_attr_t * spec_attr element on the lv_obj_t?

@C47D C47D marked this pull request as ready for review November 24, 2021 23:50
@C47D C47D marked this pull request as draft November 26, 2021 05:57
@C47D C47D marked this pull request as ready for review November 26, 2021 06:12
Properly wait for 50ms after clicking on the switch using lv_test_indev_wait and also assert on switch state after the first and second clicks
@C47D
Copy link
Contributor Author

C47D commented Nov 26, 2021

Ready to be reviewed @kisvegabor @embeddedt

lv_coord_t pad = 6;
lv_coord_t actual = 0;
// expected = 8; pad + SWITCH_ROUNDING_ERROR (2)
lv_coord_t expected = 8;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we move LV_SWITCH_KNOB_ROUNDING_ERROR to the header? I'd rather have it be visible than use a magic number.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we could, let's see @kisvegabor opinion

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally, there shouldn't be a rounding errors here, it was just a dirty fix from me.

I suggest calling it _LV_SWITCH_KNOB_EXT_AREA_CORRECTION. The _ is to indicate its note for the users.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can I place it in the header so the symbol can be used in the test?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, and please also add a meaningful comment 🙂

@C47D
Copy link
Contributor Author

C47D commented Dec 2, 2021

@kisvegabor does this change looks ok?

diff --git a/src/widgets/lv_switch.c b/src/widgets/lv_switch.c
index 2da69d067..4362f8eb6 100644
--- a/src/widgets/lv_switch.c
+++ b/src/widgets/lv_switch.c
@@ -33,9 +33,6 @@
 /** Mark no animation is in progress*/
 #define LV_SWITCH_ANIM_STATE_INV   -1

-/** Switch rounding error for know calculation */
-#define LV_SWITCH_KNOB_ROUNDING_ERROR   2
-
 /**********************
  *      TYPEDEFS
  **********************/
@@ -130,7 +127,7 @@ static void lv_switch_event(const lv_obj_class_t * class_p, lv_event_t * e)

         /*The smaller size is the knob diameter*/
         lv_coord_t knob_size = LV_MAX4(knob_left, knob_right, knob_bottom, knob_top);
-        knob_size += LV_SWITCH_KNOB_ROUNDING_ERROR;
+        knob_size += _LV_SWITCH_KNOB_EXT_AREA_CORRECTION;
         knob_size += lv_obj_calculate_ext_draw_size(obj, LV_PART_KNOB);

         lv_coord_t * s = lv_event_get_param(e);
diff --git a/src/widgets/lv_switch.h b/src/widgets/lv_switch.h
index 7d573f2af..9d6b796b7 100644
--- a/src/widgets/lv_switch.h
+++ b/src/widgets/lv_switch.h
@@ -23,6 +23,9 @@ extern "C" {
  *      DEFINES
  *********************/

+/** Switch knob extra area correction factor */
+#define _LV_SWITCH_KNOB_EXT_AREA_CORRECTION 2
+
 /**********************
  *      TYPEDEFS
  **********************/
diff --git a/tests/src/test_cases/test_switch.c b/tests/src/test_cases/test_switch.c
index fdbdd056a..666849cde 100644
--- a/tests/src/test_cases/test_switch.c
+++ b/tests/src/test_cases/test_switch.c
@@ -102,8 +102,7 @@ void test_switch_should_update_extra_draw_size_after_editing_padding(void)
 {
     lv_coord_t pad = 6;
     lv_coord_t actual = 0;
-    // expected = 8; pad + SWITCH_ROUNDING_ERROR (2)
-    lv_coord_t expected = 8;
+    lv_coord_t expected = pad + _LV_SWITCH_KNOB_EXT_AREA_CORRECTION;

     static lv_style_t style_knob;
     lv_style_init(&style_knob);

@C47D
Copy link
Contributor Author

C47D commented Dec 7, 2021

Ready to be reviewed/merged

So we can use it when testing extra draw size
Copy link
Member

@kisvegabor kisvegabor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thank you very much!

@kisvegabor kisvegabor merged commit 559c2cd into lvgl:master Dec 7, 2021
@C47D C47D deleted the tests/switch branch December 7, 2021 13:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants