From patchwork Sun Mar 16 19:10:00 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Daniel Vetter X-Patchwork-Id: 3839941 Return-Path: X-Original-To: patchwork-intel-gfx@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork2.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.19.201]) by patchwork2.web.kernel.org (Postfix) with ESMTP id 241D4BF540 for ; Sun, 16 Mar 2014 19:31:59 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id 2F5F4201F5 for ; Sun, 16 Mar 2014 19:31:58 +0000 (UTC) Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) by mail.kernel.org (Postfix) with ESMTP id 31F9F20200 for ; Sun, 16 Mar 2014 19:31:57 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id AA66A6E195; Sun, 16 Mar 2014 12:31:56 -0700 (PDT) X-Original-To: intel-gfx@lists.freedesktop.org Delivered-To: intel-gfx@lists.freedesktop.org Received: from mail-ee0-f45.google.com (mail-ee0-f45.google.com [74.125.83.45]) by gabe.freedesktop.org (Postfix) with ESMTP id 08BD86E192 for ; Sun, 16 Mar 2014 12:31:54 -0700 (PDT) Received: by mail-ee0-f45.google.com with SMTP id d17so3346942eek.18 for ; Sun, 16 Mar 2014 12:31:54 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ffwll.ch; s=google; h=from:to:cc:subject:date:message-id:in-reply-to:references; bh=RQCv4HjyqoieU77nYiVEhcP1vBAbcdp+nlLj6/30tNc=; b=jaUrx9i2eSOsklkpdfwZDUcou6qggiXuot14GKtucH7JHFmWU8c+gc+CtmXQvwycQ0 eBpUJibKk/zrv3y2RLfJvIfyMGSFP7URMaPO/Ra9+4UHbmjE4pWiINw1WsGz5SxOehhc m225N8lwzZ4Am656tkfB+iuBY3D1keePan7JQ= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:from:to:cc:subject:date:message-id:in-reply-to :references; bh=RQCv4HjyqoieU77nYiVEhcP1vBAbcdp+nlLj6/30tNc=; b=MZz7NJBpI5glfaB8H1JwtBHR7NFfHl/gknc77+eldb6rboXoj6Be87QjDjPtzup85r FG0H1P86cMP+mwGb8PQ/5rqmXd0+/bTq88BbkOE018UvJcXWsZ9lr1dc53mFTVRwd3Ai ZFWfnlyR2KCmciclq0U8ggkSXR2W+LPdpdRJ6d5h0u5IDhgDdl/KkNkMW3TbDK8ij9Ku fzwCzQt/S8YHvhy76AXMsCjgq52s5HgUZxHlAD1qGbJU/NF3sBHv5iVfWZPbi2nLR6io bo0j18g+0rIwru/0X7ZHdoP770IQT5SNhHTCFe/zBxG04IJZ0kpIDd4KCqi2o1wOKdkd YYHw== X-Gm-Message-State: ALoCoQncMbb1/52TJFsmWXxuu7YD/MzDRbXTRNalf9IEkLIBE1OfiSljFZu1FP5ZxnP6XwhcUIYH X-Received: by 10.14.110.199 with SMTP id u47mr3511104eeg.74.1394998314282; Sun, 16 Mar 2014 12:31:54 -0700 (PDT) Received: from wespe.ffwll.local (84-73-67-144.dclient.hispeed.ch. [84.73.67.144]) by mx.google.com with ESMTPSA id 48sm34246059eee.2.2014.03.16.12.31.52 for (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Sun, 16 Mar 2014 12:31:53 -0700 (PDT) From: Daniel Vetter To: Intel Graphics Development Date: Sun, 16 Mar 2014 20:10:00 +0100 Message-Id: <1394997000-3022-3-git-send-email-daniel.vetter@ffwll.ch> X-Mailer: git-send-email 1.8.1.4 In-Reply-To: <1394997000-3022-1-git-send-email-daniel.vetter@ffwll.ch> References: <1394997000-3022-1-git-send-email-daniel.vetter@ffwll.ch> Cc: Daniel Vetter Subject: [Intel-gfx] [PATCH 3/3] lib/igt_core: Document library design best practices X-BeenThere: intel-gfx@lists.freedesktop.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: Intel graphics driver community testing & development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , MIME-Version: 1.0 Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" X-Spam-Status: No, score=-4.1 required=5.0 tests=BAYES_00,DKIM_SIGNED, RCVD_IN_DNSWL_MED, T_DKIM_INVALID, T_RP_MATCHES_RCVD, UNPARSEABLE_RELAY autolearn=unavailable version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP This is what I've been doing in the past few months when refactoring i-g-t code. More ideas and also patterns to add highly welcome. Signed-off-by: Daniel Vetter --- lib/igt_core.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 49 insertions(+) diff --git a/lib/igt_core.c b/lib/igt_core.c index d8a44193a7b7..b8329cbca7ff 100644 --- a/lib/igt_core.c +++ b/lib/igt_core.c @@ -115,6 +115,55 @@ * - "they are local to the function that made the corresponding setjmp() call; * - "their values are changed between the calls to setjmp() and longjmp(); and * - "they are not declared as volatile." + * + * # Best Practices for Test Helper Libraries Design + * + * Kernel tests itself tend to have fairly complex logic already. It is + * therefore paramount that helper code, both in libraries and test-private + * function, add as little boilerplate code to the main test logic as possible. + * But then dense code is hard to understand without constantly consulting + * documentation and implementation of all the helper functions if it doesn't + * follow some clear patterns. Hence follow these established best practices: + * + * - Make extensive use of the implicit control flow afforded by igt_skip(), + * igt_fail and igt_success(). When dealing with optional kernel features + * combine igt_skip() with igt_fail() to skip when the kernel support isn't + * available but fail when anything else goes awry. void should be the most + * common return type in all your functions. + * + * - Main test logic should have no explicit control flow for failure + * conditions, but instead such assumptions should be written in a declarative + * style. Use one of the many macros which encapsulate i-g-t's implicit + * control flow. Pick the most suitable one to have as much debug output as + * possible, without polluting the code unecessarily. For example + * igt_assert_cmpint() for comparing integers or do_ioctl() for running ioctls + * and checking their results. Feel free to add new ones to the libary or + * wrap up a set of checks into a private function to further condense your + * test logic. + * + * - When adding a new feature test function which uses igt_skip() internally, + * use the <prefix>_require_<feature_name> naming scheme. When you + * instead add a feature test function which returns a boolean, because your + * main test logic must take different actions depending upon the feature's + * availability, then instead use the <prefix>_has_<feature_name>. + * + * - As already mentioned eschew explicit error handling logic as much as + * possible. If your test absolutely has to handle the error of some function + * the customary naming pattern is to prefix those variants with __. Try to + * restrict explicit error handling to leaf functions. For the main test flow + * simply pass the expected error condition down into your helper code, which + * results in tidy and declarative test logic. + * + * - Make your library functions as simple to use as possible. Automatically + * register cleanup handlers through igt_install_exit_handler(). Reduce the + * amount of setup boilerplate needed by using implicit singletons and lazy + * structure initialization and similar design patterns as much as possible. + * + * - Don't shy away from refactoring common code, even when there are just 2-3 + * users and even if it's not a net reduction in code. As long as it helps to + * remove boilerplate and makes the code more declarative the resulting + * clearer test flow is worth it. All i-g-t library code has been organically + * extracted from testcases in this fashion. */ static unsigned int exit_handler_count;