<html><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><br><div><div>On 16 Mar 2010, at 18:51, Albrecht Dreß wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><div>Am 16.03.10 10:30 schrieb(en) Ton Voon:<br><blockquote type="cite"><blockquote type="cite">What should be the purpose of a test case if the code strictly follows POSIX?</blockquote></blockquote><blockquote type="cite">Because the test cases proves real-world usage, rather than some spec?<br></blockquote><br>I don't think that POSIX and ISO 9899 are just "some spec". If my proper usage of a well-defined library function produces weird results, I wouldn't assume that I am right and all those IEEE and ISO people (and glibc coders) are wrong. The usual approach IMHO would be looking at *my* code, which might be /slightly/ less perfect at second glance...<br></div></blockquote><div><br></div>I'm not disputing that the spec is what we should work to, but the proper test has to be how Nagios handles the scenario with known inputs (configuration + time) and outputs (whether the timeperiod matches or not).</div><div><br></div><div>t-tap/test_timeperiods.c says that, for the specific case of this bit of code in base/utils.c:</div><div><br></div><div><div> /* calculate the start of the day (midnight, 00:00 hours) when the specified test time occurs */</div><div> t->tm_sec=0;</div><div> t->tm_min=0;</div><div> t->tm_hour=0;</div><div> /* Removed for the moment. This fixes a bug where the timeperiod is incorrectly calculated */</div><div> /* See t-tap/test_timeperiods for a test failure */</div><div> /* t->tm_isdst=-1; */</div><div><br></div><div>that there are failures when running test_timeperiods when tm_isdst is set to -1. You can argue with me if the test is wrong (I don't think so), but if the test is right, then setting to -1 leads to errors. Try it and see!</div><div><br></div></div><div><blockquote type="cite"><div><blockquote type="cite">What I don't have, are tests for the other 18 cases where is_dst has been initialised, and so I didn't revert those other changes. But my opinion is now that this is too cautious and we should just revert back to Nagios pre-3.2.0 behaviour.<br></blockquote><br>If you don't believe me, run the attached little C test app which takes the current time, simulates your approach of the unitialised field, and converts back, which should of course give the initial value. For me (CET, no dst in effect) the code always fails (==produces an off-by-one-hour error) if the tm_isdst is > 0. I don't think you will find any box with time zone != UTC where it doesn't...<br></div></blockquote><div><br></div>That test is irrelevant because you are testing the spec, not testing Nagios behaviour. If you can create or update a test like test_timeperiods.c which actually tests the functions within Nagios, that would be relevant.</div><div><br></div><div>Here's what I found from looking at the DST bug last year:</div><div><br></div><div> 1. Nagios, on startup, works out the offset in seconds from the top of the day based on the timeperiods in the configuration file</div><div> 2. When calculating if a timeperiod matches, it takes the current top of the day and the current offset seconds from the top and sees if it falls within the top and bottom of the offsets</div><div> 3. The bug happens on the day of the DST (going backwards) because there are 25 hours in that day, so from 11pm onwards, this does not fit in the "offset of that day" algorithm</div><div><br></div><div>test_timeperiod.c is testing the inputs and outputs of the check_time_against_period function. Arguably, the logic should be different, but if you want to refactor, then you will still need the tests to prove that you have handled all situations.</div><div><br></div><div>(In case you can't tell, I'm a testing fanatic!)</div><div><br></div><div>Now, I'm not disputing that there is some other bug that you have found, but I am saying that a wholesale change of isdst=-1 is causing other problems. I've proven the single case, but I haven't written the tests for the other 16 cases when isdst is set. </div><div><br></div><div>Hence my position: I didn't want to change the others without tests, but I think that is now too conservative and I suggest we change all of them back until the proof goes the other way - you prove why it SHOULD be isdst=-1 with appropriate test updates to test_timeperiod.c</div><div><br></div><div>My guess: you are correct that isdst needs to be set when doing calculations based on actual date ranges (such as "<span class="Apple-style-span" style="font-family: monospace; white-space: pre; "><font class="Apple-style-span" face="Helvetica"><span class="Apple-style-span" style="white-space: normal;">2007-01-01 - 2008-02-01 09:00-17:00", </span></font><span class="Apple-style-span" style="font-family: Helvetica; white-space: normal; ">rather than "mon 00:00-24:00" type format). But again, I want tests updated rather than arguments about how the spec works.</span></span></div><div><br></div><div>I think I'm right in saying that some other implementations of Nagios have reverted the entire patch for isdst=-1. Anyone care to chip in here?</div><div><br></div><div>Information about the test suite: <a href="http://wiki.nagios.org/index.php/Nagios_Core_Developer_Guidelines#Testing">http://wiki.nagios.org/index.php/Nagios_Core_Developer_Guidelines#Testing</a></div><div><br></div><div>Ton</div><div><br></div><div><br></div><div><br></div><div><br></div></body></html>