search

linux-audit / audit-testsuite

A simple, self-contained regression test suite for the Linux Kernel's audit subsystem
GNU General Public License v2.0
21 stars 25 forks source link

tests/filter_sessionid: provide more information upon failure #69

Closed rgbriggs closed 5 years ago

rgbriggs commented 6 years ago

Add one more check for success of test command and add more details of the failure when the debug flag is on.

Signed-off-by: Richard Guy Briggs rgb@redhat.com

pcmoore commented 5 years ago

This is not a complicated test, I don't see the value in these additional checks. Dropping this PR.

rgbriggs commented 5 years ago

On 2018-11-20 19:30, Paul Moore wrote:

This is not a complicated test, I don't see the value in these additional checks. Dropping this PR.

Maybe that says something about me since it was complicated enough that I had to instrument it to find out how it failed and it seemed worthwhile keeping them. These changes don't change the output when everything is working fine but most immediately help when things don't work. What is the harm in including this information for the cases when it doesn't work as expected? How do you debug when a test fails?

pcmoore commented 5 years ago

What is the harm in including this information for the cases when it doesn't work as expected?

It adds noise to the test.

How do you debug when a test fails?

You dig into the test and step through it. If you've got a failing test odds are you are going to have to do that anyway, regardless of how much info the test spews onto your terminal.

rgbriggs commented 5 years ago

On 2018-11-21 07:07, Paul Moore wrote:

What is the harm in including this information for the cases when it doesn't work as expected?

It adds noise to the test.

Noise? Not when it is working. And when it isn't, it provides information that helps diagnose it without having to step through it.

How do you debug when a test fails?

You dig into the test and step through it. If you've got a failing test odds are you are going to have to do that anyway, regardless of how much info the test spews onto your terminal.

Frequently stepping through the test doesn't reproduce the problem or causes others due to differences in environment or timing due to manual execution. A number of times it has been factors not related to the test, which were obvious from the comments on the end of the "ok" line. I wouldn't go to the trouble of instrumenting the test if it were that easy.

pcmoore commented 5 years ago

Noise? Not when it is working. And when it isn't, it provides information that helps diagnose it without having to step through it.

It adds noise to the code. It clutters up a rather simple test for, IMHO, little benefit. Don't forget I've never been a big fan of a lot of the debugging you've been adding to the tests; in some cases I concede and merge the changes, in others I drop the patch.

I understand that you disagree in this particular case - that's fine - but as of right now I'm not going to merge this commit. Period.