From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: kunit: Add backtrace suppression self-tests Date: Thu, 23 Apr 2026 09:52:26 +1000 Message-ID: In-Reply-To: <20260420-kunit_add_support-v7-3-e8bc6e0f70de@redhat.com> References: <20260420-kunit_add_support-v7-0-e8bc6e0f70de@redhat.com> <20260420-kunit_add_support-v7-3-e8bc6e0f70de@redhat.com> X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit MIME-Version: 1.0 Patch Review **Asymmetric skip conditions between WARN_ON direct and indirect tests** ```c +static void backtrace_suppression_test_warn_on_direct(struct kunit *test) +{ + if (!IS_ENABLED(CONFIG_DEBUG_BUGVERBOSE) && !IS_ENABLED(CONFIG_KALLSYMS)) + kunit_skip(test, "requires CONFIG_DEBUG_BUGVERBOSE or CONFIG_KALLSYMS"); ``` vs. ```c +static void backtrace_suppression_test_warn_on_indirect(struct kunit *test) +{ + if (!IS_ENABLED(CONFIG_DEBUG_BUGVERBOSE)) + kunit_skip(test, "requires CONFIG_DEBUG_BUGVERBOSE"); ``` The direct test accepts either `CONFIG_DEBUG_BUGVERBOSE` or `CONFIG_KALLSYMS`, while the indirect test requires `CONFIG_DEBUG_BUGVERBOSE` only. There's no comment explaining why `CONFIG_KALLSYMS` is sufficient for the direct case but not the indirect case. A brief comment would help future readers understand the distinction. **No test for allocation failure path** There's no test that exercises the case where `__kunit_start_suppress_warning()` returns NULL (allocation failure). While hard to trigger in practice, it would verify the NULL-safety of the END/COUNT macros. **Test coverage is otherwise solid** The five test cases (WARN direct/indirect, WARN_ON direct/indirect, multi-warning) cover the key scenarios well. --- Generated by Claude Code Patch Reviewer