On Thu, 13 Apr 2023 at 21:15, Michal Wajdeczko michal.wajdeczko@intel.com wrote:
On 13.04.2023 08:31, David Gow wrote:
On Thu, 13 Apr 2023 at 04:28, Rae Moar rmoar@google.com wrote:
On Tue, Apr 11, 2023 at 12:01 PM Michal Wajdeczko michal.wajdeczko@intel.com wrote:
There is function to report status of either suite or test, but it doesn't support parameterized subtests that have to log report on its own. Extend it to also accept subtest level results to avoid code duplication.
Signed-off-by: Michal Wajdeczko michal.wajdeczko@intel.com Cc: David Gow davidgow@google.com
lib/kunit/test.c | 28 +++++++++++++++++----------- 1 file changed, 17 insertions(+), 11 deletions(-)
diff --git a/lib/kunit/test.c b/lib/kunit/test.c index 5679197b5f8a..692fce258c5b 100644 --- a/lib/kunit/test.c +++ b/lib/kunit/test.c @@ -154,8 +154,14 @@ static void kunit_print_suite_start(struct kunit_suite *suite) kunit_suite_num_test_cases(suite)); }
+enum kunit_test_or_suite {
KUNIT_SUITE = 0,KUNIT_TEST,KUNIT_SUBTEST,+};
Hi Michal!
Since KUnit's goal is to progress toward supporting arbitrary levels of testing, I like the idea of starting to adjust these helper functions to allow for greater levels of testing.
However, I'm not sure about this kunit_test_or_suite enum. If our goal is to support an arbitrary number of levels of tests then this enum still limits us to a finite number of levels. However, if we only want to focus on supporting parameterized tests (which is our direct goal), this could be the right solution.
Maybe instead we could use an integer denoting the test level instead? This would remove the limit but would also remove the nice names of the levels.
we can use integer as param but at the same time we can also have define or anonymous enum as nice aliases to currently known/used levels:
/* Currently supported test levels */ enum { KUNIT_LEVEL_SUITE = 0, KUNIT_LEVEL_TEST, KUNIT_LEVEL_PARAMTEST, };
/* Future levels are TBD */ #define KUNIT_LEVEL_SUBTEST(n) (KUNIT_LEVEL_TEST + (n))
This sounds good to me!
I'm curious what others opinions are on these ideas?
A bit of a nit: if we do use this enum I wonder if we could clarify the name to be kunit_test_level as the current name of kunit_test_or_suite seems to indicate to me a binary of two options rather than three.
static void kunit_print_ok_not_ok(void *test_or_suite,
bool is_test,
enum kunit_test_or_suite is_test,Similar to above, I think the name of is_test could be clarified. It is currently a bit confusing to me as they are all tests. Maybe test_level?
ok
I agree with Rae that this is not the ideal long-term solution.
We're basically encoding two things here:
- Are we dealing with a 'struct kunit_suite' or a 'struct kunit'?
- How nested the test is.
Given KUnit originally only had a 2-level nesting (suite->test), and now has 3-level nesting (always suite->test[->param]), this works, but the KTAP format permits arbitrary nesting of tests, and we'll want to have something like that in KUnit going forward. We don't have a design for that yet, but it could conceivably allow nested suites, thus breaking the rule that nesting level 0 is always a suite, and the rest are all tests.
I guess "We don't have a design for that yet" is a key here
Yeah: I wouldn't worry too much about what the future design would bring, tbh. As long as we don't totally paint ourselves into a corner (and I don't think anything in this patch is at risk of doing that), then doing whatever is sensible for the way things are now works.
So there's definitely a part of me that would prefer to pass those two pieces of information in separate arguments, rather than relying on an enum like this.
That being said, this is all very fuzzy future plans, rather than anything concrete, and this will all likely need reworking if we do anything drastic anyway, so I'm not worried if we go with this for now, and change it when we need to. I do think it's an improvement over what we're doing currently. (For example, another possible implementation for nested tests could be to get rid of the distinction between tests and suites completely: or at least have them share 'struct kunit', so this wouldn't need passing in separately.)
maybe the problem will go away once we just replace this untyped param:
kunit_print_ok_not_ok(void *test_or_suite, ...with proper type:
kunit_print_ok_not_ok(struct kunit *test, ...and treat NULL as indication that we just want to print raw results (as it looks function is not using any suite attributes directly, all input is passed explicitly and kunit_log() expects test only)
Yeah: I think that makes sense. Suite results are still handled separately in debugfs as well.
Cheers, -- David