2015-08-31 20:37 GMT+02:00 Tomas Heinrich <[email protected]>: > Morning, > > I've been getting some random test-suite failures for some time now so I've > started looking at diag.sh and there seems to be a bunch of things that > should be fixed. > If I don't get an outright disapproval for the following analysis, I'll > start filing PRs. > > - diag.sh is sometimes sourced, sometimes not > > sample[0]: $srcdir/diag.sh wait-startup $3 || . ./diag.sh error-exit $? > > Looks like diag.sh is _not_ sourced for just couple of commands, but I think > that is coincidence and a bug; the commands still call error-exit, which is > intended to kill the test, which can't happen if diag.sh is not sourced. > I.e.: diag.sh should be always sourced, right?
I, too, think so. don't see a reason why it isnt. > > > - redundant calls to error-exit > > same sample[0] > > error-exit is already called in the command whose success is tested. > wait-startup either returns 0 or error-exit is called within. The second > part of the line is redundant. > (Maybe the need for the second part was motivated by not sourcing diag.sh in > the first part?.. But there are cases even with sourced diag.sh) I don't remember why it is there, but probably could be related to not being sourced. All of this has undergone a long evolution and many changes... > > > - inconsistent use of $srcdir vs relative path > > same sample[0] (boy, this line is bad :]) > > I guess all the occurrances of ./diag.sh should be changed to > $srcdir/diag.sh. I think so. I know that we have issues when running under "make distcheck", maybe this was a try.. maybe not. Still, "make distcheck" *has* problems these are just hidden by turning off most test when running under it. While you are at it, it would be great if you could also have a look into distcheck. > > > - The whole code that handles CURRENT_TEST is unnecessary > > The information stored in this variable / file is only ever used in > error-exit. > The information is accessible in its scope and never modified, hence it > doesn't need to be stored beforehand. The file doesn't function as a lock, > as it is truncated before spawning additional processes that could access > it. Ah don'T be too fast here. This is related to the auto, testing, which I have temporarily turned off for most part because it was useful only in some cases, fewer than where it hurt. If ever is time, I want to introduce a switch to turn this auto-debugging on and off. > > > - spawning an interactive shell > > sample[1]: > echo "ABORT! core file exists, starting interactive shell" > bash > > I think this is a mistake. Michael mentioned some time ago that he intends > to run the test-suite during package builds and I'd like to do the same. > Even in other scenarios, you probably want to keep the whole process as > non-interactive as possible. I guess this dates back to the very early days of the testbench. Full ack for changing. > > > - wait-shutdown doesn't kill the running instance > > sample[2]: echo "manual cleanup." > > After experimenting with the test-suite, I've sometimes wondered why I have > so many rsyslog instances running... :] > I think it would be cleaner to just kill the current instance after the > timeout. Leaving the instance running can mess up all subsequent tests. > same here a PR would be appreciated :-) Rainer > > Comments? > > HTH, > Tomas > > > [0] https://github.com/rsyslog/rsyslog/blob/master/tests/diag.sh#L106 > [1] https://github.com/rsyslog/rsyslog/blob/master/tests/diag.sh#L162 > [2] https://github.com/rsyslog/rsyslog/blob/master/tests/diag.sh#L156 > _______________________________________________ > rsyslog mailing list > http://lists.adiscon.net/mailman/listinfo/rsyslog > http://www.rsyslog.com/professional-services/ > What's up with rsyslog? Follow https://twitter.com/rgerhards > NOTE WELL: This is a PUBLIC mailing list, posts are ARCHIVED by a myriad of > sites beyond our control. PLEASE UNSUBSCRIBE and DO NOT POST if you DON'T > LIKE THAT. _______________________________________________ rsyslog mailing list http://lists.adiscon.net/mailman/listinfo/rsyslog http://www.rsyslog.com/professional-services/ What's up with rsyslog? Follow https://twitter.com/rgerhards NOTE WELL: This is a PUBLIC mailing list, posts are ARCHIVED by a myriad of sites beyond our control. PLEASE UNSUBSCRIBE and DO NOT POST if you DON'T LIKE THAT.
