[Linux-kernel-mentees] [RFC v3 1/2] kunit: Support for Parameterized Testing

[Linux-kernel-mentees] [RFC v3 1/2] kunit: Support for Parameterized Testing

[Arpitha Raghunandan]

Implementation of support for parameterized testing in KUnit.

Signed-off-by: Arpitha Raghunandan <98.arpi@gmail.com>
---
Changes v2->v3:
- Support for test to run iterating through all parameters added
  within the KUnit framework
- The index of the parameter causing test failure is displayed

Changes v1->v2:
- Parameterized input stored in a void* array
- An iterator method to access the different parameters

 include/kunit/test.h | 29 +++++++++++++++++++++++++++++
 lib/kunit/test.c     | 44 +++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 72 insertions(+), 1 deletion(-)

diff --git a/include/kunit/test.h b/include/kunit/test.h
index 59f3144f009a..8e9325b29058 100644
--- a/include/kunit/test.h
+++ b/include/kunit/test.h
@@ -140,10 +140,14 @@ struct kunit;
 struct kunit_case {
 	void (*run_case)(struct kunit *test);
 	const char *name;
+	void* (*get_params)(void);
+	int max_parameters_count;
+	int parameter_size;
 
 	/* private: internal use only. */
 	bool success;
 	char *log;
+	bool parameterized;
 };
 
 static inline char *kunit_status_to_string(bool status)
@@ -162,6 +166,11 @@ static inline char *kunit_status_to_string(bool status)
  */
 #define KUNIT_CASE(test_name) { .run_case = test_name, .name = #test_name }
 
+#define KUNIT_CASE_PARAM(test_name, getparams, count, size) {	.run_case = test_name,						\
+								.name = #test_name, .parameterized = true,			\
+								.get_params = (void* (*)(void))getparams,			\
+								.max_parameters_count = count, .parameter_size = size }
+
 /**
  * struct kunit_suite - describes a related collection of &struct kunit_case
  *
@@ -206,6 +215,23 @@ struct kunit {
 	/* private: internal use only. */
 	const char *name; /* Read only after initialization! */
 	char *log; /* Points at case log after initialization */
+	bool parameterized; /* True for parameterized tests */
+	/* param_values stores the test parameters
+	 * for parameterized tests.
+	 */
+	void *param_values;
+	/* max_parameters_count indicates maximum number of
+	 * parameters for parameterized tests.
+	 */
+	int max_parameters_count;
+	/* iterator_count is used by the iterator method
+	 * for parameterized tests.
+	 */
+	int iterator_count;
+	/* parameter_size indicates size of a single test case
+	 * for parameterized tests.
+	 */
+	int parameter_size;
 	struct kunit_try_catch try_catch;
 	/*
 	 * success starts as true, and may only be set to false during a
@@ -225,6 +251,7 @@ struct kunit {
 };
 
 void kunit_init_test(struct kunit *test, const char *name, char *log);
+void kunit_init_param_test(struct kunit *test, struct kunit_case *test_case);
 
 int kunit_run_tests(struct kunit_suite *suite);
 
@@ -237,6 +264,8 @@ int __kunit_test_suites_init(struct kunit_suite **suites);
 
 void __kunit_test_suites_exit(struct kunit_suite **suites);
 
+void *get_test_case_parameters(struct kunit *test);
+
 /**
  * kunit_test_suites() - used to register one or more &struct kunit_suite
  *			 with KUnit.
diff --git a/lib/kunit/test.c b/lib/kunit/test.c
index c36037200310..ae012e65368e 100644
--- a/lib/kunit/test.c
+++ b/lib/kunit/test.c
@@ -142,6 +142,11 @@ unsigned int kunit_test_case_num(struct kunit_suite *suite,
 }
 EXPORT_SYMBOL_GPL(kunit_test_case_num);
 
+static void kunit_print_failed_param(struct kunit *test)
+{
+	kunit_err(test, "\n\tTest failed at parameter: %d\n", test->iterator_count);
+}
+
 static void kunit_print_string_stream(struct kunit *test,
 				      struct string_stream *stream)
 {
@@ -182,6 +187,9 @@ static void kunit_fail(struct kunit *test, struct kunit_assert *assert)
 
 	assert->format(assert, stream);
 
+	if (test->parameterized)
+		kunit_print_failed_param(test);
+
 	kunit_print_string_stream(test, stream);
 
 	WARN_ON(string_stream_destroy(stream));
@@ -236,6 +244,18 @@ void kunit_init_test(struct kunit *test, const char *name, char *log)
 }
 EXPORT_SYMBOL_GPL(kunit_init_test);
 
+void kunit_init_param_test(struct kunit *test, struct kunit_case *test_case)
+{
+	spin_lock_init(&test->lock);
+	INIT_LIST_HEAD(&test->resources);
+	test->parameterized = true;
+	test->param_values = (void *)(test_case->get_params());
+	test->max_parameters_count = test_case->max_parameters_count;
+	test->parameter_size = test_case->parameter_size;
+	test->iterator_count = 0;
+}
+EXPORT_SYMBOL_GPL(kunit_init_param_test);
+
 /*
  * Initializes and runs test case. Does not clean up or do post validations.
  */
@@ -254,7 +274,14 @@ static void kunit_run_case_internal(struct kunit *test,
 		}
 	}
 
-	test_case->run_case(test);
+	if (!test->parameterized)
+		test_case->run_case(test);
+	else {
+		int i;
+
+		for (i = 0; i < test->max_parameters_count; i++)
+			test_case->run_case(test);
+	}
 }
 
 static void kunit_case_internal_cleanup(struct kunit *test)
@@ -343,6 +370,8 @@ static void kunit_run_case_catch_errors(struct kunit_suite *suite,
 	struct kunit test;
 
 	kunit_init_test(&test, test_case->name, test_case->log);
+	if (test_case->parameterized)
+		kunit_init_param_test(&test, test_case);
 	try_catch = &test.try_catch;
 
 	kunit_try_catch_init(try_catch,
@@ -407,6 +436,19 @@ void __kunit_test_suites_exit(struct kunit_suite **suites)
 }
 EXPORT_SYMBOL_GPL(__kunit_test_suites_exit);
 
+/*
+ * Iterator method for the parameterized test cases
+ */
+void *get_test_case_parameters(struct kunit *test)
+{
+	int index = test->iterator_count * test->parameter_size;
+
+	if (test->iterator_count != test->max_parameters_count)
+		test->iterator_count++;
+	return (test->param_values + index);
+}
+EXPORT_SYMBOL_GPL(get_test_case_parameters);
+
 /*
  * Used for static resources and when a kunit_resource * has been created by
  * kunit_alloc_resource().  When an init function is supplied, @data is passed
-- 
2.25.1

_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

[Linux-kernel-mentees] [RFC v3 2/2] fs: ext4: Modify inode-test.c to use KUnit parameterized testing

[Arpitha Raghunandan]

Modifies fs/ext4/inode-test.c to use the parameterized testing
feature of KUnit.

Signed-off-by: Arpitha Raghunandan <98.arpi@gmail.com>
---
 fs/ext4/inode-test.c | 69 +++++++++++++++++++++++++++-----------------
 1 file changed, 42 insertions(+), 27 deletions(-)

diff --git a/fs/ext4/inode-test.c b/fs/ext4/inode-test.c
index d62d802c9c12..e262fef505b3 100644
--- a/fs/ext4/inode-test.c
+++ b/fs/ext4/inode-test.c
@@ -72,6 +72,8 @@
 #define UPPER_BOUND_NONNEG_EXTRA_BITS_1_CASE\
 	"2446-05-10 Upper bound of 32bit >=0 timestamp. All extra sec bits on"
 
+#define NUMBER_OF_TESTCASES 16
+
 struct timestamp_expectation {
 	const char *test_case_name;
 	struct timespec64 expected;
@@ -101,7 +103,39 @@ static time64_t get_32bit_time(const struct timestamp_expectation * const test)
  */
 static void inode_test_xtimestamp_decoding(struct kunit *test)
 {
-	const struct timestamp_expectation test_data[] = {
+	struct timespec64 timestamp;
+
+	struct timestamp_expectation *test_case =
+		(struct timestamp_expectation *)get_test_case_parameters(test);
+
+	timestamp.tv_sec = get_32bit_time(test_case);
+	ext4_decode_extra_time(&timestamp,
+			       cpu_to_le32(test_case->extra_bits));
+
+	KUNIT_EXPECT_EQ_MSG(test,
+			    test_case->expected.tv_sec,
+			    timestamp.tv_sec,
+			    CASE_NAME_FORMAT,
+			    test_case->test_case_name,
+			    test_case->msb_set,
+			    test_case->lower_bound,
+			    test_case->extra_bits);
+	KUNIT_EXPECT_EQ_MSG(test,
+			    test_case->expected.tv_nsec,
+			    timestamp.tv_nsec,
+			    CASE_NAME_FORMAT,
+			    test_case->test_case_name,
+			    test_case->msb_set,
+			    test_case->lower_bound,
+			    test_case->extra_bits);
+}
+
+struct timestamp_expectation *get_test_parameters(void)
+{
+	struct timestamp_expectation *test_data = (struct timestamp_expectation *)
+		kmalloc(sizeof(struct timestamp_expectation) * NUMBER_OF_TESTCASES, GFP_KERNEL);
+
+	const struct timestamp_expectation test_data_init[] = {
 		{
 			.test_case_name = LOWER_BOUND_NEG_NO_EXTRA_BITS_CASE,
 			.msb_set = true,
@@ -232,35 +266,16 @@ static void inode_test_xtimestamp_decoding(struct kunit *test)
 		}
 	};
 
-	struct timespec64 timestamp;
-	int i;
-
-	for (i = 0; i < ARRAY_SIZE(test_data); ++i) {
-		timestamp.tv_sec = get_32bit_time(&test_data[i]);
-		ext4_decode_extra_time(&timestamp,
-				       cpu_to_le32(test_data[i].extra_bits));
-
-		KUNIT_EXPECT_EQ_MSG(test,
-				    test_data[i].expected.tv_sec,
-				    timestamp.tv_sec,
-				    CASE_NAME_FORMAT,
-				    test_data[i].test_case_name,
-				    test_data[i].msb_set,
-				    test_data[i].lower_bound,
-				    test_data[i].extra_bits);
-		KUNIT_EXPECT_EQ_MSG(test,
-				    test_data[i].expected.tv_nsec,
-				    timestamp.tv_nsec,
-				    CASE_NAME_FORMAT,
-				    test_data[i].test_case_name,
-				    test_data[i].msb_set,
-				    test_data[i].lower_bound,
-				    test_data[i].extra_bits);
-	}
+	memcpy(test_data, test_data_init,
+		sizeof(struct timestamp_expectation) * ARRAY_SIZE(test_data_init));
+
+	return test_data;
 }
 
 static struct kunit_case ext4_inode_test_cases[] = {
-	KUNIT_CASE(inode_test_xtimestamp_decoding),
+	KUNIT_CASE_PARAM(inode_test_xtimestamp_decoding,
+			get_test_parameters, NUMBER_OF_TESTCASES,
+			sizeof(struct timestamp_expectation)),
 	{}
 };
 
-- 
2.25.1

_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Re: [Linux-kernel-mentees] [RFC v3 1/2] kunit: Support for Parameterized Testing

[Brendan Higgins via Linux-kernel-mentees]

+Marco Elver

Marco, just figured you might want to see and comment on the current
state of parameterized testing.

Arpitha, can you add Marco on to future revisions?

On Sat, Oct 3, 2020 at 7:50 AM Arpitha Raghunandan <98.arpi@gmail.com> wrote:
>
> Implementation of support for parameterized testing in KUnit.
>
> Signed-off-by: Arpitha Raghunandan <98.arpi@gmail.com>
> ---
> Changes v2->v3:
> - Support for test to run iterating through all parameters added
>   within the KUnit framework
> - The index of the parameter causing test failure is displayed
>
> Changes v1->v2:
> - Parameterized input stored in a void* array
> - An iterator method to access the different parameters
>
>  include/kunit/test.h | 29 +++++++++++++++++++++++++++++
>  lib/kunit/test.c     | 44 +++++++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 72 insertions(+), 1 deletion(-)
>
> diff --git a/include/kunit/test.h b/include/kunit/test.h
> index 59f3144f009a..8e9325b29058 100644
> --- a/include/kunit/test.h
> +++ b/include/kunit/test.h
> @@ -140,10 +140,14 @@ struct kunit;
>  struct kunit_case {
>         void (*run_case)(struct kunit *test);
>         const char *name;
> +       void* (*get_params)(void);
> +       int max_parameters_count;
> +       int parameter_size;
>
>         /* private: internal use only. */
>         bool success;
>         char *log;
> +       bool parameterized;
>  };
>
>  static inline char *kunit_status_to_string(bool status)
> @@ -162,6 +166,11 @@ static inline char *kunit_status_to_string(bool status)
>   */
>  #define KUNIT_CASE(test_name) { .run_case = test_name, .name = #test_name }
>
> +#define KUNIT_CASE_PARAM(test_name, getparams, count, size) {  .run_case = test_name,                                          \
> +                                                               .name = #test_name, .parameterized = true,                      \
> +                                                               .get_params = (void* (*)(void))getparams,                       \
> +                                                               .max_parameters_count = count, .parameter_size = size }
> +
>  /**
>   * struct kunit_suite - describes a related collection of &struct kunit_case
>   *
> @@ -206,6 +215,23 @@ struct kunit {
>         /* private: internal use only. */
>         const char *name; /* Read only after initialization! */
>         char *log; /* Points at case log after initialization */
> +       bool parameterized; /* True for parameterized tests */
> +       /* param_values stores the test parameters
> +        * for parameterized tests.
> +        */
> +       void *param_values;
> +       /* max_parameters_count indicates maximum number of
> +        * parameters for parameterized tests.
> +        */
> +       int max_parameters_count;
> +       /* iterator_count is used by the iterator method
> +        * for parameterized tests.
> +        */
> +       int iterator_count;
> +       /* parameter_size indicates size of a single test case
> +        * for parameterized tests.
> +        */
> +       int parameter_size;
>         struct kunit_try_catch try_catch;
>         /*
>          * success starts as true, and may only be set to false during a
> @@ -225,6 +251,7 @@ struct kunit {
>  };
>
>  void kunit_init_test(struct kunit *test, const char *name, char *log);
> +void kunit_init_param_test(struct kunit *test, struct kunit_case *test_case);
>
>  int kunit_run_tests(struct kunit_suite *suite);
>
> @@ -237,6 +264,8 @@ int __kunit_test_suites_init(struct kunit_suite **suites);
>
>  void __kunit_test_suites_exit(struct kunit_suite **suites);
>
> +void *get_test_case_parameters(struct kunit *test);
> +
>  /**
>   * kunit_test_suites() - used to register one or more &struct kunit_suite
>   *                      with KUnit.
> diff --git a/lib/kunit/test.c b/lib/kunit/test.c
> index c36037200310..ae012e65368e 100644
> --- a/lib/kunit/test.c
> +++ b/lib/kunit/test.c
> @@ -142,6 +142,11 @@ unsigned int kunit_test_case_num(struct kunit_suite *suite,
>  }
>  EXPORT_SYMBOL_GPL(kunit_test_case_num);
>
> +static void kunit_print_failed_param(struct kunit *test)
> +{
> +       kunit_err(test, "\n\tTest failed at parameter: %d\n", test->iterator_count);
> +}
> +
>  static void kunit_print_string_stream(struct kunit *test,
>                                       struct string_stream *stream)
>  {
> @@ -182,6 +187,9 @@ static void kunit_fail(struct kunit *test, struct kunit_assert *assert)
>
>         assert->format(assert, stream);
>
> +       if (test->parameterized)
> +               kunit_print_failed_param(test);
> +
>         kunit_print_string_stream(test, stream);
>
>         WARN_ON(string_stream_destroy(stream));
> @@ -236,6 +244,18 @@ void kunit_init_test(struct kunit *test, const char *name, char *log)
>  }
>  EXPORT_SYMBOL_GPL(kunit_init_test);
>
> +void kunit_init_param_test(struct kunit *test, struct kunit_case *test_case)
> +{
> +       spin_lock_init(&test->lock);
> +       INIT_LIST_HEAD(&test->resources);
> +       test->parameterized = true;
> +       test->param_values = (void *)(test_case->get_params());
> +       test->max_parameters_count = test_case->max_parameters_count;
> +       test->parameter_size = test_case->parameter_size;
> +       test->iterator_count = 0;
> +}
> +EXPORT_SYMBOL_GPL(kunit_init_param_test);
> +
>  /*
>   * Initializes and runs test case. Does not clean up or do post validations.
>   */
> @@ -254,7 +274,14 @@ static void kunit_run_case_internal(struct kunit *test,
>                 }
>         }
>
> -       test_case->run_case(test);
> +       if (!test->parameterized)
> +               test_case->run_case(test);
> +       else {
> +               int i;
> +
> +               for (i = 0; i < test->max_parameters_count; i++)
> +                       test_case->run_case(test);
> +       }
>  }
>
>  static void kunit_case_internal_cleanup(struct kunit *test)
> @@ -343,6 +370,8 @@ static void kunit_run_case_catch_errors(struct kunit_suite *suite,
>         struct kunit test;
>
>         kunit_init_test(&test, test_case->name, test_case->log);
> +       if (test_case->parameterized)
> +               kunit_init_param_test(&test, test_case);
>         try_catch = &test.try_catch;
>
>         kunit_try_catch_init(try_catch,
> @@ -407,6 +436,19 @@ void __kunit_test_suites_exit(struct kunit_suite **suites)
>  }
>  EXPORT_SYMBOL_GPL(__kunit_test_suites_exit);
>
> +/*
> + * Iterator method for the parameterized test cases
> + */
> +void *get_test_case_parameters(struct kunit *test)
> +{
> +       int index = test->iterator_count * test->parameter_size;
> +
> +       if (test->iterator_count != test->max_parameters_count)
> +               test->iterator_count++;
> +       return (test->param_values + index);
> +}
> +EXPORT_SYMBOL_GPL(get_test_case_parameters);
> +
>  /*
>   * Used for static resources and when a kunit_resource * has been created by
>   * kunit_alloc_resource().  When an init function is supplied, @data is passed
> --
> 2.25.1
>
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Re: [Linux-kernel-mentees] [RFC v3 2/2] fs: ext4: Modify inode-test.c to use KUnit parameterized testing

[Brendan Higgins via Linux-kernel-mentees]

Arpitha, please add Iurii on future revisions. He authored this test.

On Sat, Oct 3, 2020 at 7:51 AM Arpitha Raghunandan <98.arpi@gmail.com> wrote:
>
> Modifies fs/ext4/inode-test.c to use the parameterized testing
> feature of KUnit.
>
> Signed-off-by: Arpitha Raghunandan <98.arpi@gmail.com>
> ---
>  fs/ext4/inode-test.c | 69 +++++++++++++++++++++++++++-----------------
>  1 file changed, 42 insertions(+), 27 deletions(-)
>
> diff --git a/fs/ext4/inode-test.c b/fs/ext4/inode-test.c
> index d62d802c9c12..e262fef505b3 100644
> --- a/fs/ext4/inode-test.c
> +++ b/fs/ext4/inode-test.c
> @@ -72,6 +72,8 @@
>  #define UPPER_BOUND_NONNEG_EXTRA_BITS_1_CASE\
>         "2446-05-10 Upper bound of 32bit >=0 timestamp. All extra sec bits on"
>
> +#define NUMBER_OF_TESTCASES 16
> +
>  struct timestamp_expectation {
>         const char *test_case_name;
>         struct timespec64 expected;
> @@ -101,7 +103,39 @@ static time64_t get_32bit_time(const struct timestamp_expectation * const test)
>   */
>  static void inode_test_xtimestamp_decoding(struct kunit *test)
>  {
> -       const struct timestamp_expectation test_data[] = {
> +       struct timespec64 timestamp;
> +
> +       struct timestamp_expectation *test_case =

I think test_data or test_parameters makes more sense.

> +               (struct timestamp_expectation *)get_test_case_parameters(test);
> +
> +       timestamp.tv_sec = get_32bit_time(test_case);
> +       ext4_decode_extra_time(&timestamp,
> +                              cpu_to_le32(test_case->extra_bits));
> +
> +       KUNIT_EXPECT_EQ_MSG(test,
> +                           test_case->expected.tv_sec,
> +                           timestamp.tv_sec,
> +                           CASE_NAME_FORMAT,
> +                           test_case->test_case_name,
> +                           test_case->msb_set,
> +                           test_case->lower_bound,
> +                           test_case->extra_bits);
> +       KUNIT_EXPECT_EQ_MSG(test,
> +                           test_case->expected.tv_nsec,
> +                           timestamp.tv_nsec,
> +                           CASE_NAME_FORMAT,
> +                           test_case->test_case_name,
> +                           test_case->msb_set,
> +                           test_case->lower_bound,
> +                           test_case->extra_bits);
> +}
> +
> +struct timestamp_expectation *get_test_parameters(void)
> +{
> +       struct timestamp_expectation *test_data = (struct timestamp_expectation *)
> +               kmalloc(sizeof(struct timestamp_expectation) * NUMBER_OF_TESTCASES, GFP_KERNEL);

I don't see this get freed anywhere; you can get around it with
kunit_kmalloc. However, I suspect you won't need this at all given my
next comment...

> +
> +       const struct timestamp_expectation test_data_init[] = {

Can't you just make the scope of this array global or static and then
just return a pointer to an element in the array?

>                 {
>                         .test_case_name = LOWER_BOUND_NEG_NO_EXTRA_BITS_CASE,
>                         .msb_set = true,
> @@ -232,35 +266,16 @@ static void inode_test_xtimestamp_decoding(struct kunit *test)
>                 }
>         };
>
> -       struct timespec64 timestamp;
> -       int i;
> -
> -       for (i = 0; i < ARRAY_SIZE(test_data); ++i) {
> -               timestamp.tv_sec = get_32bit_time(&test_data[i]);
> -               ext4_decode_extra_time(&timestamp,
> -                                      cpu_to_le32(test_data[i].extra_bits));
> -
> -               KUNIT_EXPECT_EQ_MSG(test,
> -                                   test_data[i].expected.tv_sec,
> -                                   timestamp.tv_sec,
> -                                   CASE_NAME_FORMAT,
> -                                   test_data[i].test_case_name,
> -                                   test_data[i].msb_set,
> -                                   test_data[i].lower_bound,
> -                                   test_data[i].extra_bits);
> -               KUNIT_EXPECT_EQ_MSG(test,
> -                                   test_data[i].expected.tv_nsec,
> -                                   timestamp.tv_nsec,
> -                                   CASE_NAME_FORMAT,
> -                                   test_data[i].test_case_name,
> -                                   test_data[i].msb_set,
> -                                   test_data[i].lower_bound,
> -                                   test_data[i].extra_bits);
> -       }
> +       memcpy(test_data, test_data_init,
> +               sizeof(struct timestamp_expectation) * ARRAY_SIZE(test_data_init));
> +
> +       return test_data;
>  }
>
>  static struct kunit_case ext4_inode_test_cases[] = {
> -       KUNIT_CASE(inode_test_xtimestamp_decoding),
> +       KUNIT_CASE_PARAM(inode_test_xtimestamp_decoding,
> +                       get_test_parameters, NUMBER_OF_TESTCASES,
> +                       sizeof(struct timestamp_expectation)),
>         {}
>  };
>
> --
> 2.25.1
>
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Re: [Linux-kernel-mentees] [RFC v3 1/2] kunit: Support for Parameterized Testing

[Brendan Higgins via Linux-kernel-mentees]

On Sat, Oct 3, 2020 at 7:50 AM Arpitha Raghunandan <98.arpi@gmail.com> wrote:
[...]
> @@ -254,7 +274,14 @@ static void kunit_run_case_internal(struct kunit *test,
>                 }
>         }
>
> -       test_case->run_case(test);
> +       if (!test->parameterized)
> +               test_case->run_case(test);
> +       else {

Sorry, I just caught this. When you put either the if or else block in
braces; the other should go in braces as well.

> +               int i;
> +
> +               for (i = 0; i < test->max_parameters_count; i++)
> +                       test_case->run_case(test);
> +       }
>  }
>
[...]
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Re: [Linux-kernel-mentees] [RFC v3 2/2] fs: ext4: Modify inode-test.c to use KUnit parameterized testing

[Arpitha Raghunandan]

On 09/10/20 11:50 pm, Brendan Higgins wrote:
> Arpitha, please add Iurii on future revisions. He authored this test.
> Okay!

> On Sat, Oct 3, 2020 at 7:51 AM Arpitha Raghunandan <98.arpi@gmail.com> wrote:
>>
>> Modifies fs/ext4/inode-test.c to use the parameterized testing
>> feature of KUnit.
>>
>> Signed-off-by: Arpitha Raghunandan <98.arpi@gmail.com>
>> ---
>>  fs/ext4/inode-test.c | 69 +++++++++++++++++++++++++++-----------------
>>  1 file changed, 42 insertions(+), 27 deletions(-)
>>
>> diff --git a/fs/ext4/inode-test.c b/fs/ext4/inode-test.c
>> index d62d802c9c12..e262fef505b3 100644
>> --- a/fs/ext4/inode-test.c
>> +++ b/fs/ext4/inode-test.c
>> @@ -72,6 +72,8 @@
>>  #define UPPER_BOUND_NONNEG_EXTRA_BITS_1_CASE\
>>         "2446-05-10 Upper bound of 32bit >=0 timestamp. All extra sec bits on"
>>
>> +#define NUMBER_OF_TESTCASES 16
>> +
>>  struct timestamp_expectation {
>>         const char *test_case_name;
>>         struct timespec64 expected;
>> @@ -101,7 +103,39 @@ static time64_t get_32bit_time(const struct timestamp_expectation * const test)
>>   */
>>  static void inode_test_xtimestamp_decoding(struct kunit *test)
>>  {
>> -       const struct timestamp_expectation test_data[] = {
>> +       struct timespec64 timestamp;
>> +
>> +       struct timestamp_expectation *test_case =
> 
> I think test_data or test_parameters makes more sense.
> 
>> +               (struct timestamp_expectation *)get_test_case_parameters(test);
>> +
>> +       timestamp.tv_sec = get_32bit_time(test_case);
>> +       ext4_decode_extra_time(&timestamp,
>> +                              cpu_to_le32(test_case->extra_bits));
>> +
>> +       KUNIT_EXPECT_EQ_MSG(test,
>> +                           test_case->expected.tv_sec,
>> +                           timestamp.tv_sec,
>> +                           CASE_NAME_FORMAT,
>> +                           test_case->test_case_name,
>> +                           test_case->msb_set,
>> +                           test_case->lower_bound,
>> +                           test_case->extra_bits);
>> +       KUNIT_EXPECT_EQ_MSG(test,
>> +                           test_case->expected.tv_nsec,
>> +                           timestamp.tv_nsec,
>> +                           CASE_NAME_FORMAT,
>> +                           test_case->test_case_name,
>> +                           test_case->msb_set,
>> +                           test_case->lower_bound,
>> +                           test_case->extra_bits);
>> +}
>> +
>> +struct timestamp_expectation *get_test_parameters(void)
>> +{
>> +       struct timestamp_expectation *test_data = (struct timestamp_expectation *)
>> +               kmalloc(sizeof(struct timestamp_expectation) * NUMBER_OF_TESTCASES, GFP_KERNEL);
> 
> I don't see this get freed anywhere; you can get around it with
> kunit_kmalloc. However, I suspect you won't need this at all given my
> next comment...
> 
>> +
>> +       const struct timestamp_expectation test_data_init[] = {
> 
> Can't you just make the scope of this array global or static and then
> just return a pointer to an element in the array?
> 
Yes, this is better. I will make this change.

>>                 {
>>                         .test_case_name = LOWER_BOUND_NEG_NO_EXTRA_BITS_CASE,
>>                         .msb_set = true,
>> @@ -232,35 +266,16 @@ static void inode_test_xtimestamp_decoding(struct kunit *test)
>>                 }
>>         };
>>
>> -       struct timespec64 timestamp;
>> -       int i;
>> -
>> -       for (i = 0; i < ARRAY_SIZE(test_data); ++i) {
>> -               timestamp.tv_sec = get_32bit_time(&test_data[i]);
>> -               ext4_decode_extra_time(&timestamp,
>> -                                      cpu_to_le32(test_data[i].extra_bits));
>> -
>> -               KUNIT_EXPECT_EQ_MSG(test,
>> -                                   test_data[i].expected.tv_sec,
>> -                                   timestamp.tv_sec,
>> -                                   CASE_NAME_FORMAT,
>> -                                   test_data[i].test_case_name,
>> -                                   test_data[i].msb_set,
>> -                                   test_data[i].lower_bound,
>> -                                   test_data[i].extra_bits);
>> -               KUNIT_EXPECT_EQ_MSG(test,
>> -                                   test_data[i].expected.tv_nsec,
>> -                                   timestamp.tv_nsec,
>> -                                   CASE_NAME_FORMAT,
>> -                                   test_data[i].test_case_name,
>> -                                   test_data[i].msb_set,
>> -                                   test_data[i].lower_bound,
>> -                                   test_data[i].extra_bits);
>> -       }
>> +       memcpy(test_data, test_data_init,
>> +               sizeof(struct timestamp_expectation) * ARRAY_SIZE(test_data_init));
>> +
>> +       return test_data;
>>  }
>>
>>  static struct kunit_case ext4_inode_test_cases[] = {
>> -       KUNIT_CASE(inode_test_xtimestamp_decoding),
>> +       KUNIT_CASE_PARAM(inode_test_xtimestamp_decoding,
>> +                       get_test_parameters, NUMBER_OF_TESTCASES,
>> +                       sizeof(struct timestamp_expectation)),
>>         {}
>>  };
>>
>> --
>> 2.25.1
>>

_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Re: [Linux-kernel-mentees] [RFC v3 1/2] kunit: Support for Parameterized Testing

[Arpitha Raghunandan]

On 09/10/20 11:53 pm, Brendan Higgins wrote:
> On Sat, Oct 3, 2020 at 7:50 AM Arpitha Raghunandan <98.arpi@gmail.com> wrote:
> [...]
>> @@ -254,7 +274,14 @@ static void kunit_run_case_internal(struct kunit *test,
>>                 }
>>         }
>>
>> -       test_case->run_case(test);
>> +       if (!test->parameterized)
>> +               test_case->run_case(test);
>> +       else {
> 
> Sorry, I just caught this. When you put either the if or else block in
> braces; the other should go in braces as well.
> 

Okay, I'll fix this.

>> +               int i;
>> +
>> +               for (i = 0; i < test->max_parameters_count; i++)
>> +                       test_case->run_case(test);
>> +       }
>>  }
>>
> [...]
> 

_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Re: [Linux-kernel-mentees] [RFC v3 2/2] fs: ext4: Modify inode-test.c to use KUnit parameterized testing

[Iurii Zaikin via Linux-kernel-mentees]

Thanks for modernizing the test!

> >> +#define NUMBER_OF_TESTCASES 16
If you move the test case array outside the function body, you won't
need this because you'll be able to use ARRAY_SIZE.
Although it could be argued that the get_test_case_parameters
interface should provide the array length to go with the array itself.


> >> +struct timestamp_expectation *get_test_parameters(void)
static?
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees