 |
History
| Log In
|
|
|
|
Ok, I'm using the pt_PT locale. As it seems, it doesn't start weeks on Sunday... http://www-950.ibm.com/software/globalization/icu/demo/locales/?d_=en&_=pt_PT says it starts at day 2 (what??)... Anyway, i believe the unit tests should assume a specific locale, no?
Thanks for all the info, Pedro I think it's more that NthIncludedDayTrigger's shouldn't be used in locales without Sunday as a start day. So the unit test is doing the right thing and showing a problem in the source.
At the minimum we should document this limitation/issue in the javadoc of the trigger.
We should also fix the test so you can build in a different locale; even if the Trigger is useless in said locale.
Scratch the 3->TriggerUtils.TUESDAY btw. The 3 means the 3rd included day, if the calendar provided says to ignore a holiday, it's possible that the 3rd day might be a wednesday etc.
Maybe Calendar.getFirstDayOfWeek can be used to solve this. First step, need to setup a test to replicate Pedro's problem.
Assuming the first day of the week is Sunday or Monday, the value of N for a weekly trigger is:
calendarDay - firstDayOfWeek + 1 where calendarDay is a Calendar.xxxDAY constant, and firstDayOfWeek is found by Calendar.getInstance().getFirstDayOfWeek(). Attaching quartz-519-01.diff which uses this method in the tests to determine the value of N that should be used for the expected target dates. (more to follow) Another approach is to set the default locale which is used when running the tests.
If all the NthIncludedDayTrigger tests run in the same locale, then this sort of problem shouldn't occur. The only problem might be that setting the locale might not work in an environment with certain security manager settings (but do people run tests like that?) Attaching another diff (an alternative to the first) that sets the locale in the test setup, and restores it in the tear down. Again, this passes for me on a machine with getFirstDayOfWeek = 2. Last one for now...
This one removes the obligation on the client code to deal with the firstDayOfWeek of the current locale, by providing a setDayOfWeek(int) method, in addition to setN(int). Hence, for a weekly trigger, the client can call setDayOfWeek(java.util.Calendar.xxxDAY), and not worry about which day the current locale regards as being the first day in the week. It only works for weekly triggers - as demonstrated by the tests. Again, this is an alternative to the previous two diffs. Hope one of these is of some help... Whew! You guys have certainly found quite a complicated one!
It definitely appears that the problem is here (starting at line 776) //move to the first day of the week (SUNDAY) currCal.add(java.util.Calendar.DAY_OF_MONTH, (afterCal.get(java.util.Calendar.DAY_OF_WEEK) - 1) * -1); This code is assuming that the first day of the week has a value of 0, which evidently is not true. When writing this (and indeed, before I saw this entry) I had no idea there were places in the world that didn't start their weeks on the same day... Anyhow, the point of the code above is to start checking dates on the first day of the week prior to the "afterDate". I think if we modify it to be sensitive to the first day of the week, we should be okay. That code might be a bit awkward to write though. Basically we need to figure out how many days to back up, but without relying on the values of the Calendar.DAY_OF_WEEK constants. Calendar.SUNDAY has the value 1, but if Monday is the first day of the week, we actually need to subtract 6 days (as though Calendar.SUNDAY == 7). Now, I just hope that there aren't timezones that do something strange like start months on day 2 or have December as the first month of the year... Oops... This line above:
This code is assuming that the first day of the week has a value of 0, which evidently is not true. should read This code is assuming that the first day of the week has a value of 1, which evidently is not always true. Okay, last comment tonight, I promise. I'm attaching a fourth patch. I think it corrects the problem, but I'm not sure how to set up a test, so I'll need someone to help with that. The solution I used is hardly elegant, but I think it will do the trick. I'd appreciate it if this was fairly heavily tested, though...
The fourth patch doesn't work here - i.e. the tests still fail.
What's the behaviour for weekly triggers supposed to be? Is the value you pass to setN(int) supposed to be one of Calendar.xxxDAY? Or is it supposed to be the number of days since the start of the week? My second patch might give you an idea of how to set up a test for how it will behave in a different locale. Also, could someone look at the hunk from AnnualCalendarTest from my patches, please? That test also still fails. Thanks. "Is the value you pass to setN(int) supposed to be one of Calendar.xxxDAY? Or is it supposed to be the number of days since the start of the week?" Just in case it's not obvious, the third patch allows the user to pick which of these two behaviours they want. Err... neither, really...
NthIncludedDayTrigger has no real use without a Calendar. When interval type is INTERVAL_TYPE_WEEKLY a value N indicates that the trigger should fire: -on the nth day following the first day of the week that is not excluded by the associated calendar. So, an example (using next week as an example): N = 3, My calendar excludes 4/1/2008, In my timezone, the first day of the week is Sunday (3/30/2008) The trigger should fire on 4/2/2008. The 3rd day of the week is actually 4/1/2008, but my Calendar excluded that date. Thus the name NthINCLUDEDDayTrigger. In the absence of a Calendar, the trigger would indeed default to the Nth day of the week. It SHOULD be the Nth day after the first day of the week, which means in my example above, if the first day of the week is Monday (3/31/2008), the trigger should fire on (4/3/2008). I'm not sure if the tests or the trigger are working correctly for a different timezone, but that is the expected behavior. I'll try to take a closer look soon. Ah, yes. I understand what you mean. It's been so long since I looked at the issue that I'd forgotten what the class was about.
I think "on the nth day following the first day of the week that is not excluded by the associated calendar" clarifies the desired behaviour: you _do_ want the behaviour to vary, according to which day the locale defines as being the first in the week. In that case, I don't see how this can be fixed without changing the tests, since they use constant 'N' and then assertEquals using specific dates. The test could be fixed by: * varying N according to the current locale to suit the expected date (patch 1); or * varying the expected date according to the current locale to suit N; or * setting the default locale to use in the tests (patch 2). I'm not sure whether there are any bugs in the code itself - I'm struggling to understand it. However, I'd be rather more confident that the tests are in error, based on your explanation above. As far as I can tell, the code did have a bug, in that it was relying on the constant values to help "calculate" the first day of the week. This presupposed that the first day of the week had the value of 1, which it does if the first day of the week is Calendar.SUNDAY. But that is not always true. So my patch corrects this (though not exactly in an elegant fasion).
Now I need to look at the test. I believe you are correct that the test needs to vary according to the local timezone (or at a minimum, test in multiple timezones). I would prefer to vary the expected date, rather than vary N, as it semantically matches what we're about to test better. Varying N feels rather like rigging the test to give me the answer I want, rather than ensuring that the answer I get is correct. More to come... Henri, with your permission, I think I'm going to take over this entry... at least temporarily. Okay, both the trigger and the test were wrong, with the test not expecting the correct behavior, and with the trigger not exhibiting the correct behavior. Please apply this patch AND patch
This works in my timezone (America/New_York), and presumeably, in any timezone where java.util.Calendar.getFirstDayOfWeek() returns java.util.Calendar.SUNDAY. I need to know if it works in locales where this is not true Haha! And I just realized all of our patches are named incorrectly... they all point to
Okay, I lied. I didn't wait on Henri to give permission. You can yell at me later, if you'd like, Henri :-)
I'm raising the priority on this to Major, as it will make the trigger work incorrectly in some timezones. It's not a blocker, but I think it's a definite must before the next release.
It's all yours. I always upload my latest status on issues, so never worry about diving in and fixing things instead :)
OK, I'll try and test this tonight... perhaps late afternoon in your locale ;o)
I'll also file the AnnualCalendarTest problem separately, if it's not already been done. With patches 4 and 5:
testGetFireTimeAfter() passes, but testTimeZone() fails. Presumably testTimeZone() was also failing before. It's the same kind of problem. Perhaps it's worth surrounding these tests in something like: Locale saved = Locale.getDefault(); Locale[] testLocales = new Locale[] { Locale.US, Locale.UK }; for (int i = 0; i < testLocales.length; i++) { Locale.setDefault(testLocales[i]); // test body } Locale.setDefault(saved); to avoid regressions? Ugggg... I don't know enough about how TimeZones work to figure this one out. As far as I know, the Trigger is behaving correctly now, but I can't figure out what this test is doing. It's working exclusively with GMT and EST, which both indicate that the first day of the week is Sunday, so I don't see how this test is failing... But as I said, I don't understand the timezone stuff.
Going to request James or one of the other developers take a look. The first day of week is determined by the locale - not the timezone. So for example, here the first day of week is Monday for GMT.
If you specify both timezone and locale to Calendar.getInstance() then everyone will get the same result, regardless of which locale is the default. If you don't specify, then the default locale is used, and we get different results (unless you set the default locale yourself). If you try surrounding the test in the previous snippet, and tracing through it in the debugger, you'll see what I mean. Thank you for your explanation Martin, it helped me immensely (though I must say the timezone stuff still throws me for a loop).
I'm attaching To recap, I'm asking you to test based on the current SVN contents + And I *reaallly* hope this one's right :-) Aaron, Sorry for the delay.
We changed to daylight savings time on 31 March, and at work have been dealing with a number of bugs that came to light after that. So I just couldn't bear to look at this bug during the week! Anyway, I've just tested it now and it seems fine. *Phew!* Guess this can be closed now. Thanks. Wonderful! I'm going to get at least one of the other developers to review my work before I commit it, due to the complexity here and my own lack of knowledge regarding Quartz's timezone support. After that, I'll commit and resolve the issue.
Thank you very much for all your help and time with this one, Martin. It's very much appreciated. Correction to Trigger. This brings us up to patch 6 for the tests and patch 7 for the trigger.
Committed changes (finally). See notes for more information
| ||||||||||||||||||||||||||||||||||||||||||||||||||||
 |
|
Bug
Closed
Major
NthIncludedDayTriggerTest won't pass
I'm running on OS X, timezone = PST, Java = 1.5.0_06, Ant 1.7.0, Locale is US_ENGLISH (as far as I know).
As a minor improvement, I imagine the test should be changed to say:
weeklyTrigger.setN( TriggerUtils.TUESDAY )
What's the locale/timezone for the machine you're running this on? Maybe that might provide a clue. I'm especially thinking in terms of the first day of the week being a different day than it is in US_ENGLISH - line 774 of NthIncludedDayTrigger says "//move to the first day of the week (SUNDAY)" and if your first day is MONDAY then that would make sense of the need to switch from 3 to 2 above.
Another option is that the javadoc talks about holidays and weekends being skipped - in your locale the holiday may clash with the unit test; however I think the first day of week reason above is most likely.