Fixing DST cutover bug in test cases #119
Conversation
|
SimianArmy-pull-requests #125 SUCCESS |
|
SimianArmy-pull-requests #126 SUCCESS |
|
In the last commit, I created a TestUtils class, removed a lot of the copy/paste utility code peppered through the test classes, and simplified the logic of the termination date checking. Took Nir's advice to just check to see if the termination was within a day of what we expect it to be. The DST cutover only bumps it by an hour, but as you see from the earlier commit, checking those bounds adds more code and logic to the test case. I additionally wrote a unit test where the date is fixed and the test time crosses a known DST cutover boundary, to verify that the cutover occurs and that the new test logic works correctly for both instances. The other tests look like unit tests but are not "pure" unit tests as they do not mock the system time, and rely heavily on calls to the system to get the current time to populate rules, resource detach/create dates, etc. |
|
SimianArmy-pull-requests #127 SUCCESS |
|
SimianArmy-pull-requests #128 SUCCESS |
|
Looks good |
Fixing DST cutover bug in test cases
I encountered a bug when running the tests very close to the spring DST cutover.
Six tests failed because they were comparing the number of days between now and the expected termination date, using miliseconds and long datatypes. Due to the "spring ahead" effect of the DST cutover, the actual difference was not a full 4 days, but rather 3.95 (4 days minus 1 hour), so the tests failed, as the fractional portion was dropped from the long number, and 3 != 4.
I modified the tests to allow for a one-hour buffer on either side.
More (unnecessarily complicated) logic could be added to determine if the test will collide with the DST cutover at execution time, and adjust only in that case, but that seems excessive.
Alternately, this failure raises the question about whether the volume tagging and termination policies are correct. Currently, this means that if a policy is set to terminate an AWS Resource in a set number of days, it may actually do too early (by one hour, in the fall) or too late (by one hour, in the spring).
If the current behavior with a small margin of error two times a year is ok, we can leave it. Otherwise, we should retool to make it "clock-accurate" (and this raises further questions of making it user-configurable, ignoring this margin of error in states that do not observe DST, etc.).
My recommendation is that we just go with the fix to the unit tests, now that they correctly implement the "roughly" logic that is called out in the test case javadoc.