Added failing spec for looping on DST boundary #104

Merged
merged 1 commit into from Oct 16, 2012

Conversation

Projects
None yet
3 participants
@forrest

forrest commented Oct 15, 2012

Hey,

I'm stilling hitting this infinite loop, as per issue #98 or #53 (not sure if it's the same problem). Included is a failing spec to help.

Forrest

@nathany

This comment has been minimized.

Show comment Hide comment
@nathany

nathany Oct 15, 2012

Contributor

schedule#find_occurrences is caught in a loop because next_time is returning the same time it received. It is being passed in Sun, 31 Mar 2013 00:00:01 GMT +00:00 and a nil closing time.

Contributor

nathany commented Oct 15, 2012

schedule#find_occurrences is caught in a loop because next_time is returning the same time it received. It is being passed in Sun, 31 Mar 2013 00:00:01 GMT +00:00 and a nil closing time.

@forrest

This comment has been minimized.

Show comment Hide comment
@forrest

forrest Oct 15, 2012

What's happening is the DST shift is moving it forward one hour. Then the next scheduled time is always the point one hour in the future.

forrest commented Oct 15, 2012

What's happening is the DST shift is moving it forward one hour. Then the next scheduled time is always the point one hour in the future.

seejohnrun added a commit that referenced this pull request Oct 16, 2012

Merge pull request #104 from forrest/master
Added failing spec for looping on DST boundary

@seejohnrun seejohnrun merged commit 77a6057 into seejohnrun:master Oct 16, 2012

1 check failed

default The Travis build failed
Details
@seejohnrun

This comment has been minimized.

Show comment Hide comment
@seejohnrun

seejohnrun Oct 16, 2012

Owner

:( This passes for me on 0.9.0 - what is your local time zone?

Owner

seejohnrun commented Oct 16, 2012

:( This passes for me on 0.9.0 - what is your local time zone?

@forrest

This comment has been minimized.

Show comment Hide comment
@forrest

forrest Oct 16, 2012

MST

forrest commented Oct 16, 2012

MST

@forrest

This comment has been minimized.

Show comment Hide comment
@forrest

forrest Oct 16, 2012

I wrote some lower lever specs in a seperate branch on my fork: https://github.com/forrest/ice_cube/blob/d8d122d938e0dfeae0995241d8c9e047f7f652b3/spec/examples/validated_rule_spec.rb

The timezone DST edge case which only adds the 1 second fails, but adding 1 hour and 1 second passes.

forrest commented Oct 16, 2012

I wrote some lower lever specs in a seperate branch on my fork: https://github.com/forrest/ice_cube/blob/d8d122d938e0dfeae0995241d8c9e047f7f652b3/spec/examples/validated_rule_spec.rb

The timezone DST edge case which only adds the 1 second fails, but adding 1 hour and 1 second passes.

@forrest

This comment has been minimized.

Show comment Hide comment
@forrest

forrest Oct 16, 2012

@seejohnrun It's worth noting we're also seeing some other specs failing. I wonder if it's related to the version of ruby or one of the supporting gems?

forrest commented Oct 16, 2012

@seejohnrun It's worth noting we're also seeing some other specs failing. I wonder if it's related to the version of ruby or one of the supporting gems?

@forrest

This comment has been minimized.

Show comment Hide comment
@forrest

forrest Oct 16, 2012

Also worth noting: TravisCI isn't liking it one bit either. In my new branch, I have a timeout to prevent infinite loops, but the build from your merge ran for 52 minutes: https://travis-ci.org/#!/seejohnrun/ice_cube/builds/2804774

forrest commented Oct 16, 2012

Also worth noting: TravisCI isn't liking it one bit either. In my new branch, I have a timeout to prevent infinite loops, but the build from your merge ran for 52 minutes: https://travis-ci.org/#!/seejohnrun/ice_cube/builds/2804774

@seejohnrun

This comment has been minimized.

Show comment Hide comment
@seejohnrun

seejohnrun Oct 16, 2012

Owner

Okay - thanks for all of the notes
I probably won't be able to take a look until tomorrow night - but will get back to you soon

Owner

seejohnrun commented Oct 16, 2012

Okay - thanks for all of the notes
I probably won't be able to take a look until tomorrow night - but will get back to you soon

@forrest

This comment has been minimized.

Show comment Hide comment
@forrest

forrest Oct 16, 2012

haha, np. Thanks for putting up with all the spam. Have a good night.

forrest commented Oct 16, 2012

haha, np. Thanks for putting up with all the spam. Have a good night.

@seejohnrun

This comment has been minimized.

Show comment Hide comment
@seejohnrun

seejohnrun Oct 16, 2012

Owner

you too :)

Owner

seejohnrun commented Oct 16, 2012

you too :)

@forrest

This comment has been minimized.

Show comment Hide comment
@forrest

forrest Oct 16, 2012

Final message of the night I promise ;-)

I did some refactoring of the ValidatedRule#next_time method. Makes it much easier to track down what is going on. As it has the same bug, I didn't do a pull request yet, but thought it may be helpful: forrest/ice_cube@master...next_time_fix

forrest commented Oct 16, 2012

Final message of the night I promise ;-)

I did some refactoring of the ValidatedRule#next_time method. Makes it much easier to track down what is going on. As it has the same bug, I didn't do a pull request yet, but thought it may be helpful: forrest/ice_cube@master...next_time_fix

@forrest

This comment has been minimized.

Show comment Hide comment
@forrest

forrest Oct 17, 2012

Hey @seejohnrun, sorry to keep bugging you, but we do have production code relying on this. Is there anything else I can be doing to help?

forrest commented Oct 17, 2012

Hey @seejohnrun, sorry to keep bugging you, but we do have production code relying on this. Is there anything else I can be doing to help?

@seejohnrun

This comment has been minimized.

Show comment Hide comment
@seejohnrun

seejohnrun Oct 18, 2012

Owner

Hey @forrest -
I'm sorry for the delay on this fix

while I do have a strong drive to keep things going at a reasonable speed on IceCube,
I don't work at Patch where it's used - so my paying things get in the way a bit.

Any moves you can make towards a fix are helpful, but unfortunately I have some mouths to feed so I only have a few nights a week to give for free. I'll absolutely have this back to you this week

Owner

seejohnrun commented Oct 18, 2012

Hey @forrest -
I'm sorry for the delay on this fix

while I do have a strong drive to keep things going at a reasonable speed on IceCube,
I don't work at Patch where it's used - so my paying things get in the way a bit.

Any moves you can make towards a fix are helpful, but unfortunately I have some mouths to feed so I only have a few nights a week to give for free. I'll absolutely have this back to you this week

seejohnrun added a commit that referenced this pull request Oct 19, 2012

@forrest

This comment has been minimized.

Show comment Hide comment
@forrest

forrest Oct 22, 2012

That seems to fix the problem for us :-)

Mind if I do a little refactoring on this section and send a pull request? Just some cleanup, so we stand a better chance of being able to help if any future issues occur.

forrest commented Oct 22, 2012

That seems to fix the problem for us :-)

Mind if I do a little refactoring on this section and send a pull request? Just some cleanup, so we stand a better chance of being able to help if any future issues occur.

@seejohnrun

This comment has been minimized.

Show comment Hide comment
@seejohnrun

seejohnrun Oct 22, 2012

Owner

Not at all - let me know if I can help with anything!

Owner

seejohnrun commented Oct 22, 2012

Not at all - let me know if I can help with anything!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment