Re: [Qemu-devel] [PATCH v2 08/45] tests: add more int/number ranges checks

[Markus Armbruster]

Marc-André Lureau <address@hidden> writes:

> Suggested by Markus Armbruster:
>
> We should systematically cover the integers, in particular the
> boundaries (because that's where bugs like to hide):
>
> * Integers in [-2^63,0) can be visited with visit_type_int() and
>   visit_type_number().
>
> * Integers in [0,2^63) can be visited with visit_type_int(),
>   visit_type_uint64() and visit_type_number().
>
> * Integers in [2^63,2^64) can be visited with visit_type_uint64() and
>   visit_type_number().
>
> * Integers outside [-2^63,2^53) can be visited with visit_type_number().
>
> In any case, visit_type_number() loses precision beyond 53 bits.
>
> Signed-off-by: Marc-André Lureau <address@hidden>
> ---
>  tests/test-qobject-input-visitor.c | 38 
> ++++++++++++++++++++++++++++++++++++--
>  1 file changed, 36 insertions(+), 2 deletions(-)
>
> diff --git a/tests/test-qobject-input-visitor.c 
> b/tests/test-qobject-input-visitor.c
> index 83d663d11d..f2ed3161af 100644
> --- a/tests/test-qobject-input-visitor.c
> +++ b/tests/test-qobject-input-visitor.c
> @@ -102,11 +102,11 @@ static Visitor 
> *visitor_input_test_init_raw(TestInputVisitorData *data,
>  {
>      return visitor_input_test_init_internal(data, false, json_string, NULL);
>  }
> -

Whoops.

>  static void test_visitor_in_int(TestInputVisitorData *data,
>                                  const void *unused)
>  {
>      int64_t res = 0;
> +    double dbl;
>      int value = -42;
>      Visitor *v;
>  
> @@ -114,6 +114,9 @@ static void test_visitor_in_int(TestInputVisitorData 
> *data,
>  
>      visit_type_int(v, NULL, &res, &error_abort);
>      g_assert_cmpint(res, ==, value);
> +
> +    visit_type_number(v, NULL, &dbl, &error_abort);
> +    g_assert_cmpfloat(dbl, ==, -42.0);
>  }
>  
>  static void test_visitor_in_uint(TestInputVisitorData *data,
> @@ -121,6 +124,8 @@ static void test_visitor_in_uint(TestInputVisitorData 
> *data,
>  {
>      Error *err = NULL;
>      uint64_t res = 0;
> +    int64_t i64;
> +    double dbl;
>      int value = 42;
>      Visitor *v;
>  
> @@ -129,8 +134,13 @@ static void test_visitor_in_uint(TestInputVisitorData 
> *data,
>      visit_type_uint64(v, NULL, &res, &error_abort);
>      g_assert_cmpuint(res, ==, (uint64_t)value);
>  
> -    /* BUG: value between INT64_MIN and -1 accepted modulo 2^64 */
> +    visit_type_int(v, NULL, &i64, &error_abort);
> +    g_assert_cmpint(i64, ==, value);
>  
> +    visit_type_number(v, NULL, &dbl, &error_abort);
> +    g_assert_cmpfloat(dbl, ==, value);
> +
> +    /* BUG: value between INT64_MIN and -1 accepted modulo 2^64 */
>      v = visitor_input_test_init(data, "%d", -value);
>  
>      visit_type_uint64(v, NULL, &res, &error_abort);
> @@ -142,6 +152,8 @@ static void test_visitor_in_uint(TestInputVisitorData 
> *data,
>  
>      visit_type_uint64(v, NULL, &res, &err);
>      error_free_or_abort(&err);
> +
> +    visit_type_number(v, NULL, &dbl, &error_abort);
>  }
>  
>  static void test_visitor_in_int_overflow(TestInputVisitorData *data,
> @@ -260,6 +272,26 @@ static void test_visitor_in_number(TestInputVisitorData 
> *data,
>      g_assert_cmpfloat(res, ==, value);
>  }
>  
> +static void test_visitor_in_large_number(TestInputVisitorData *data,
> +                                         const void *unused)
> +{
> +    Error *err = NULL;
> +    double res = 0;
> +    int64_t i64;
> +    uint64_t u64;
> +    Visitor *v;
> +
> +    v = visitor_input_test_init(data, "-18446744073709551616"); /* -2^64 */
> +
> +    visit_type_number(v, NULL, &res, &error_abort);

Shouldn't we check res has the expected value?

> +
> +    visit_type_int(v, NULL, &i64, &err);
> +    error_free_or_abort(&err);
> +
> +    visit_type_uint64(v, NULL, &u64, &err);
> +    error_free_or_abort(&err);
> +}

Not sure this is worth its own test.  But then I'm never sure whether to
write heaps of small tests, or few larger ones.  You decide.

> +
>  static void test_visitor_in_number_keyval(TestInputVisitorData *data,
>                                            const void *unused)
>  {
> @@ -1253,6 +1285,8 @@ int main(int argc, char **argv)
>                             NULL, test_visitor_in_bool_str_fail);
>      input_visitor_test_add("/visitor/input/number",
>                             NULL, test_visitor_in_number);
> +    input_visitor_test_add("/visitor/input/large_number",
> +                           NULL, test_visitor_in_large_number);
>      input_visitor_test_add("/visitor/input/number_keyval",
>                             NULL, test_visitor_in_number_keyval);
>      input_visitor_test_add("/visitor/input/number_str_keyval",

The new tests are welcome, but they don't "systematically cover the
integers".  The easiest fix is to adjust the commit message's claims
down:

    tests: Add more int/number ranges checks

    Signed-off-by: Marc-André Lureau <address@hidden>