| Message ID | 1314021451-24808-1-git-send-email-penberg@kernel.org (mailing list archive) |
|---|---|
| State | Mainlined, archived |
| Headers | show |
On Mon, Aug 22, 2011 at 6:57 AM, Pekka Enberg <penberg@kernel.org> wrote: > This patch changes 'make check' output to show sparse output compared to > expected results upon unexpected test failure. For example, > static-forward-decl.c output would look like this if it would not be tagged as > "known to fail": I like that. Some minor suggestion: There are two files to check for, "output" and "error". "$stream" is the loop variable. You only print the last one in the loop. The "output" diff file does not show. I think we might just give the same error message for "known to fail" case as well. Yes, it is annoying to see them, but that is the point, we want to fix them. Chris -- To unsubscribe from this list: send the line "unsubscribe linux-sparse" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 8/22/11 4:57 PM, Pekka Enberg wrote: > This patch changes 'make check' output to show sparse output compared to > expected results upon unexpected test failure. For example, > static-forward-decl.c output would look like this if it would not be tagged as > "known to fail": > > TEST static forward declaration (static-forward-decl.c) > error: actual error text does not match expected error text. > --- static-forward-decl.c.error.expected 2011-08-22 06:29:40.000000000 +0000 > +++ static-forward-decl.c.error.got 2011-08-22 06:29:40.000000000 +0000 > @@ -0,0 +1 @@ > +static-forward-decl.c:3:5: warning: symbol 'f' was not declared. Should it be static? > error: see static-forward-decl.c.error.* for further investigation. > info: test 'static-forward-decl.c' is known to fail > > This makes it easier to detect and analyze test breakage. > > Cc: Christopher Li<sparse@chrisli.org> > Cc: Linus Torvalds<torvalds@linux-foundation.org> > Signed-off-by: Pekka Enberg<penberg@kernel.org> > --- > validation/test-suite | 2 ++ > 1 files changed, 2 insertions(+), 0 deletions(-) > > diff --git a/validation/test-suite b/validation/test-suite > index 42f7bd7..7549fd2 100755 > --- a/validation/test-suite > +++ b/validation/test-suite > @@ -146,6 +146,8 @@ do_test() > if [ "$?" -eq "0" ]; then > echo "info: test '$file' is known to fail" > known_ko_tests=`expr $known_ko_tests + 1` > + else > + cat "$file".$stream.diff > fi > return 1 > else Chris, the patch you committed is different from mine: http://git.kernel.org/?p=devel/sparse/chrisl/sparse.git;a=commitdiff;h=a7a00d5108c36b8baaf54814aa1f42583dabc754 Your patch now makes the runner verbose for "known to fail" tests which is definitely not something we should do. If someone tagged the test as "known to fail", we should treat it just like we treat passed test cases. The whole point of my patch was to make "make check" pinpoint *unexpected* breakage so that anyone who bothers to do "make check" on their patches can never cause regressions. Pekka -- To unsubscribe from this list: send the line "unsubscribe linux-sparse" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Aug 26, 2011 at 2:10 AM, Pekka Enberg <penberg@kernel.org> wrote: > Chris, the patch you committed is different from mine: > > http://git.kernel.org/?p=devel/sparse/chrisl/sparse.git;a=commitdiff;h=a7a00d5108c36b8baaf54814aa1f42583dabc754 > > Your patch now makes the runner verbose for "known to fail" tests > which is definitely not something we should do. If someone tagged > the test as "known to fail", we should treat it just like we treat > passed test cases. That is intentional. I haven't seen you reply on my comment so I take the liberty to change it. I do want to see the "known to fail" test case. > > The whole point of my patch was to make "make check" pinpoint > *unexpected* breakage so that anyone who bothers to do "make check" > on their patches can never cause regressions. How to you know your new patch did not break "known to fail" test case in a different way than originally? It could be regression as well. You can diff the aggregate output of "make check" before and after the new patch to see if the new patch affect any test case at all. That is better than blindly skip the "known to fail" test case. Yes, I have a different usage case in mind. I want to look at what currently breaks. In the long run, hopefully we fix all the known issues so this wouldn't be any issue at all. You can submit a patch to add a config for it if you are so insist on the "I don't care about already broken test cases" usage case. To me, diff the "make check" output is straightly better. Chris -- To unsubscribe from this list: send the line "unsubscribe linux-sparse" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 23 Aug 2011, Christopher Li wrote: > I think we might just give the same error message for > "known to fail" case as well. Yes, it is annoying to see them, > but that is the point, we want to fix them. No, people who did not break them should not need to see the errors! If you're going to have a "known to fail" flag, then it really should be silent *by default*. When I run make check, I want to know I didn't break anything. If someone flagged a test as "known to fail", I really don't need to care about it. Alternatively, we could get rid of the "known to fail" tag completely and just make sure the tests pass all the time. I actually wanted to do that but didn't have the skills to fix the remaining issues ;-) Pekka -- To unsubscribe from this list: send the line "unsubscribe linux-sparse" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, 26 Aug 2011, Christopher Li wrote: >> Your patch now makes the runner verbose for "known to fail" tests >> which is definitely not something we should do. If someone tagged >> the test as "known to fail", we should treat it just like we treat >> passed test cases. > > That is intentional. I haven't seen you reply on my comment so I take > the liberty to change it. I do want to see the "known to fail" test > case. Sorry about that. I somehow missed the email. >> The whole point of my patch was to make "make check" pinpoint >> *unexpected* breakage so that anyone who bothers to do "make check" >> on their patches can never cause regressions. > > How to you know your new patch did not break "known to fail" test > case in a different way than originally? It could be regression as well. I think that's irrelevant, really. I happen to think it's completely pointless to even run test cases that are "known to fail" because it creates confusion and adds little value. And if we really care that a "known to fail" test case fails in a certain way, we can turn it into a "known _not_ to fail" and assert the exact failure case. > You can diff the aggregate output of "make check" before and after > the new patch to see if the new patch affect any test case at all. > That is better than blindly skip the "known to fail" test case. No, that doesn't scale at all if you run "make check" every few minutes like I do. > Yes, I have a different usage case in mind. I want to look at what currently > breaks. In the long run, hopefully we fix all the known issues so this > wouldn't be any issue at all. Agreed. > You can submit a patch to add a config for it if you are so insist on > the "I don't care about already broken test cases" usage case. > To me, diff the "make check" output is straightly better. OK, a flag would definitely work for me. Pekka -- To unsubscribe from this list: send the line "unsubscribe linux-sparse" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/validation/test-suite b/validation/test-suite index 42f7bd7..7549fd2 100755 --- a/validation/test-suite +++ b/validation/test-suite @@ -146,6 +146,8 @@ do_test() if [ "$?" -eq "0" ]; then echo "info: test '$file' is known to fail" known_ko_tests=`expr $known_ko_tests + 1` + else + cat "$file".$stream.diff fi return 1 else
This patch changes 'make check' output to show sparse output compared to expected results upon unexpected test failure. For example, static-forward-decl.c output would look like this if it would not be tagged as "known to fail": TEST static forward declaration (static-forward-decl.c) error: actual error text does not match expected error text. --- static-forward-decl.c.error.expected 2011-08-22 06:29:40.000000000 +0000 +++ static-forward-decl.c.error.got 2011-08-22 06:29:40.000000000 +0000 @@ -0,0 +1 @@ +static-forward-decl.c:3:5: warning: symbol 'f' was not declared. Should it be static? error: see static-forward-decl.c.error.* for further investigation. info: test 'static-forward-decl.c' is known to fail This makes it easier to detect and analyze test breakage. Cc: Christopher Li <sparse@chrisli.org> Cc: Linus Torvalds <torvalds@linux-foundation.org> Signed-off-by: Pekka Enberg <penberg@kernel.org> --- validation/test-suite | 2 ++ 1 files changed, 2 insertions(+), 0 deletions(-)