Received: by 2002:a05:6a10:8c0a:0:0:0:0 with SMTP id go10csp2003100pxb; Fri, 29 Jan 2021 10:22:55 -0800 (PST) X-Google-Smtp-Source: ABdhPJwh5rfLBH6tj66z87Unt13BIoaf6WWhsjE87CL18jaKQbYPZEGL89d4DHpby5jrjShu6HuW X-Received: by 2002:a17:907:3345:: with SMTP id yr5mr6082762ejb.50.1611944574936; Fri, 29 Jan 2021 10:22:54 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1611944574; cv=none; d=google.com; s=arc-20160816; b=y/pwr2bkG9JsOPFlpPxTy2ccb7uJSY6ygoBkQBMznBeg0afb3ICaY1Jxl6CTCNejyT KGbH1J781XDiATRUGxEHcGL+FJ5TttILd86qgYH1YG2mSHhzt9+E5BvnrIGcfrvNH7Wi zL0wxNChaLHhb2ZeJvlQWawIpDqm63f5NFbDBDbrxY53zsMcWtNAD3JcYEEuBN2TXSnX F3InuqdyiBMby4lnLHuF/gfcx4U+JtA1qMys+tPS/jbF6z5HLrN8OclIXYPFsKwr0kYF 9eDZXp0lckOFGzQqOM1+Nw0PjCm7YvOucDtbpDWLnn1An+Av25sII4/+JUV6Y2VOwJMx 4euA== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:cc:to:subject:message-id:date:from:in-reply-to :references:mime-version:dkim-signature; bh=Dkfy9D4szoTbkUSTq/ZhsTMr1TyIJnO6J28s6U7fg4I=; b=aJJmgONa/5gXunmM3oWOob5ihGWPRVG3ywkLwiZQ0J1+XFGWs0p27D3OqUKLefmjFG nxkFR7NexrwP7hiUtoccWSGnXR0Z5SVJvplxT4h3hyVYBS1/uLQIywm6IBPBpLgAvX97 dWB5FLlvf5flPEPrxRUM9vgH6Gsie6lJ9FUOTfc0c+UWL3LCMhXgDK2+OQ8DYZeFRWVG y6v10X44eaqehrGtQlovtR+51F/NO3q5g28yxzuBRf29d3Do2AGuwMK0Qf32Yt9FNEB8 9snthVSrSla0hIktZJt3eHlJ0XfjbGDogsZZKD52Ppv3kl7yfAb2sVsJbNxUoZ4Sm7un +mZg== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@google.com header.s=20161025 header.b=CVpTeuwd; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=REJECT sp=REJECT dis=NONE) header.from=google.com Return-Path: Received: from vger.kernel.org (vger.kernel.org. [23.128.96.18]) by mx.google.com with ESMTP id g1si5618032edv.264.2021.01.29.10.22.17; Fri, 29 Jan 2021 10:22:54 -0800 (PST) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18; Authentication-Results: mx.google.com; dkim=pass header.i=@google.com header.s=20161025 header.b=CVpTeuwd; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=REJECT sp=REJECT dis=NONE) header.from=google.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S232683AbhA2SVO (ORCPT + 99 others); Fri, 29 Jan 2021 13:21:14 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:34068 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S231195AbhA2SUt (ORCPT ); Fri, 29 Jan 2021 13:20:49 -0500 Received: from mail-il1-x135.google.com (mail-il1-x135.google.com [IPv6:2607:f8b0:4864:20::135]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 2C11CC061574 for ; Fri, 29 Jan 2021 10:20:09 -0800 (PST) Received: by mail-il1-x135.google.com with SMTP id q9so9381520ilo.1 for ; Fri, 29 Jan 2021 10:20:09 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=Dkfy9D4szoTbkUSTq/ZhsTMr1TyIJnO6J28s6U7fg4I=; b=CVpTeuwdpsNzmOl9z2MSMm9h9Byz+lm1DBfCOMcC70X9z9O7pYUamk4dAzrWkVF3u1 UKBsp03itKVjhp0MZ9Z6Ajw3phF/lARtw9HTrEPY/IdfH70rWuoSQaC0otCKrrQg66Op IPjtxOWLju1Gp5FQ2FSWnXwllgb5zgz/lQTWzhEa9EeNC8zDKRBG3uWf+P7v1vWRqL6A MIbnGzHvRRlZ85WhkSebx31G8CMyB4jjJ4oRNTCNxZ5rJoUpAHWKLKQJeHThHfuymjYX FG97aGnqcKYnpp8UI0ahjSEIU3b0wINhzo9Duv6QjFnJki1Kd2pIRw0LYflTi0qLzjxU VKBA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=Dkfy9D4szoTbkUSTq/ZhsTMr1TyIJnO6J28s6U7fg4I=; b=CNsH8rfqk6QPB1a3dvFchal7QFkahQzgK+v/eY1yWqEReDL8Wic86k3JUF76bIByo0 k0Z0SUd7XwSD8dYb/PCSj1UaeHEycbSfV96bF7w4c1XU7dWQknp73ugn7SKeoeDvg0X8 ENq6nLbc9orgTqrxBq0645KT5wcA43AR4Ww9c/PrkmJNm8sECvXDfswh0dSlwqkrVFbJ IvQaGkW0FwBpXS3c7+bTD78TWuHV6rS9IworoU45cOpOlLx/fipec36M0TJ1uX50BL0J dKFxJY7l5l+xt7fpSJBCFbhUUKDIbuTewsfO/F2Sc52BydD47Wq50Hfa/TGUyOtB9ok9 PdWw== X-Gm-Message-State: AOAM5301WVLAYynnL1xJdqLHn4u0Rd8r52vp8NBPtFoXWS13V3Gg305e N7jHp6eTm2mE7nN5Fv1eA5o3rNrAdgkD01t0E+O6LQ== X-Received: by 2002:a92:510:: with SMTP id q16mr4072338ile.136.1611944408326; Fri, 29 Jan 2021 10:20:08 -0800 (PST) MIME-Version: 1.0 References: <20210129022555.2411999-1-dlatypov@google.com> In-Reply-To: From: Daniel Latypov Date: Fri, 29 Jan 2021 10:19:56 -0800 Message-ID: Subject: Re: [PATCH] kunit: don't show `1 == 1` in failed assertion messages To: David Gow Cc: Brendan Higgins , Linux Kernel Mailing List , "open list:KERNEL SELFTEST FRAMEWORK" , Shuah Khan Content-Type: text/plain; charset="UTF-8" Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, Jan 28, 2021 at 8:51 PM David Gow wrote: > > On Fri, Jan 29, 2021 at 10:26 AM Daniel Latypov wrote: > > > > Currently, given something (fairly dystopian) like > > > KUNIT_EXPECT_EQ(test, 2 + 2, 5) > > > > KUnit will prints a failure message like this. > > > Expected 2 + 2 == 5, but > > > 2 + 2 == 4 > > > 5 == 5 > > > > With this patch, the output just becomes > > > Expected 2 + 2 == 5, but > > > 2 + 2 == 4 > > > > This patch is slightly hacky, but it's quite common* to compare an > > expression to a literal integer value, so this can make KUnit less > > chatty in many cases. (This patch also fixes variants like > > KUNIT_EXPECT_GT, LE, et al.). > > > > It also allocates an additional string briefly, but given this only > > happens on test failures, it doesn't seem too bad a tradeoff. > > Also, in most cases it'll realize the lengths are unequal and bail out > > before the allocation. > > > > We could save the result of the formatted string to avoid wasting this > > extra work, but it felt cleaner to leave it as-is. > > > > Edge case: for something silly and unrealistic like > > > KUNIT_EXPECT_EQ(test, 4, 5); > > > > It'll generate this message with a trailing "but" > > > Expected 2 + 2 == 5, but > > > > > I assume this is supposed to say "Expected 4 == 5" here. > (I tested it to make sure, and that's what it did here.) Ah yes, too much copy-paste. > > Personally, I'd ideally like to get rid of the ", but", or even add a > "but 4 != 5" style second line. Particularly in case the next line in > the output might be confused for the rest of a sentence. Given the apparent interest in other types (STR_EQ) of literal ellision, maybe this should be done. But I'd be tempted to have that change come later once at least the str_eq version is in place. > > That being said, this is a pretty silly edge case: I'd be worried if > we ever saw that case in an actual submitted test. People might see it > a bit while debugging, though: particularly if they're using > KUNIT_EXPECT_EQ(test, 1, 2) as a way of forcing a test to fail. (I've > done this while testing tooling, for instance.) Same/Agreed on all points. > > > > > It didn't feel worth adding a check up-front to see if both sides are > > literals to handle this better. > > > > *A quick grep suggests 100+ comparisons to an integer literal as the > > right hand side. > > > > Signed-off-by: Daniel Latypov > > --- > > I tested this, and it works well: the results are definitely more > human readable. I could see it making things slightly more complicated > for people who wanted to automatically parse assertion errors, but > no-one is doing that, and the extra complexity is pretty minimal > anyway. Hmm, machine parsing of the contents of failures is interesting. But in general, that feels that requires a more structured format. I hate to invoke it, but the tooling I've seen that's parsed the "expected" and "actual" values has represented them as XML elements. > > One thing which might be worth doing is expanding this to > KUNIT_EXPECT_STREQ() and/or KUNIT_EXPECT_PTR_EQ(). These have slightly > more complicated formatting (quotes, leading 0s, etc), though. > Comparing pointer literals is pretty unlikely to show up, though, so I > don't think it's as important. (I thought that maybe the KASAN shadow > memory tests might use them, but a quick look didn't reveal any.) > Ack. Actually, the string literal check was smaller, see below. I debated sending a patch out for that, but this case mattered more and I wasn't sure if it would be acceptable or not. It felt it would be incongruous to only handle strings and not the much more common integer case. So if the hackier, more costly integer comparison seems fine, I might actually go and send out the str patch that I already have sitting around anyways. +/* Checks if KUNIT_EXPECT_STREQ() args were string literals. + * Note: `text` will have ""s where as `value` will not. + */ +static bool is_str_literal(const char *text, const char *value) +{ + int len; + + len = strlen(text); + if (len < 2) return false; + if (text[0] != '\"' || text[len-1] != '\"') return false; + + return strncmp(text+1, value, len-2) == 0; +} + This produces [10:05:59] Expected str == "world", but [10:05:59] str == hello One misgiving I had was whether we should "fix" the string printing to quote the values or not before adding `is_str_literal()` in. Having just "str == hello" where neither is quoted is a bit unclear and the extra "world == world" line sorta helped make that more clear, ha. David, I can send a version of this patch w/ a fixed commit message and then tack on the str changes as children. Would you prefer that? > For the record, this is what STREQ()/PTR_EQ()/ failures with literals look like: > # example_simple_test: EXPECTATION FAILED at lib/kunit/kunit-example-test.c:31 > Expected "abc" == "abd", but > "abc" == abc > "abd" == abd > # example_simple_test: EXPECTATION FAILED at lib/kunit/kunit-example-test.c:33 > Expected 0x124 == 0x1234, but > 0x124 == 0000000000000124 > 0x1234 == 0000000000001234 Yeah, I had considered PTR_EQ(), but it seemed more complex and also less likely to show up. And outside of very niche use cases (which probably don't work too on UML, tbh...), it felt like an anti-pattern to have hard-coded pointers in unit tests. > > Either way, though, this is: > > Tested-by: David Gow > > Cheers, > -- David