kevin-j-m / clockwork-test

A mechanism for testing clockwork jobs
MIT License
43 stars 16 forks source link

Redefining constants #30

Closed ce07c3 closed 6 years ago

ce07c3 commented 6 years ago

Perhaps due to the way it is requiring the clockwork configuration file:

config/clock.rb:9: warning: previous definition of EVERY_TEN_FULL_MINUTES was here
kevin-j-m commented 6 years ago

I'm not sure how this is an issue with the repo if the problem is in your clock file. Could you clarify how this is manifesting as a result of this test library?

ce07c3 commented 6 years ago

Hey @kevin-j-m! First of all, many thanks for the gem and for responding. :)

The gem does try to require the clock configuration file I assume, and for the five test cases we have, it does so five times. Everything after the first re-requires the module and its constants I believe?

kevin-j-m commented 6 years ago

Are you running Clockwork::Test.clear! before/after every run? Every time run is invoked, it will load the clock file. I would guess if you have multiple runs without a clear, then this will load the file multiple times, which I'd guess would raise that warning.

ce07c3 commented 6 years ago

Yes, the load would be the problem here. Ruby's require requires only once. Maybe the result of the load ought to be memoized in some way?

kevin-j-m commented 6 years ago

Are you clearing the manager between all of your 5 test runs? You're going to want to do that anyway to lose the job history between runs - but I believe will also resolve your issues of calling run multiple times with a given manager.

ce07c3 commented 6 years ago

How do I clear it? I can try it, but the load would still happen within the same Ruby runtime, right - not solving the multiple load?

kevin-j-m commented 6 years ago

Clockwork::Test.clear! gives you a new instance of the manager to call run against.

kevin-j-m commented 6 years ago

Did that help you out?

ce07c3 commented 6 years ago

No, that's not related to the issue with the re-loading.

kevin-j-m commented 6 years ago

Thanks for responding. I apologize for not fully understanding the issue until taking another look at it today. I have a branch with a reproduction, where the commit message explains what is happening.

Note that I believe this is another manifestation of the problems in #1. I believe that resolving that would handle this issue as well. I would like to close this in favor of tracking a single issue with the same symptoms (though I'll reference this in there). Sound good?

I welcome contributions to help resolve this.

ce07c3 commented 6 years ago

I looked into it and the way the test library monkey patches clockwork makes me wonder if it isn't wiser to build hooks into clockwork itself.

Another issue: I haven't looked into how the time gem is used, but it does interfere with e.g. the Google gems:

"Invalid JWT: Token must be a short-lived token (60 minutes) and in a reasonable timeframe. Check your iat and exp values and use a clock with skew to account for clock differences between systems."

I'll try to look into it during the week or weekend.

kevin-j-m commented 6 years ago

Clockwork::Test does use Timecop to change time if you specify what you want the time to be. However, it also returns it at the conclusion of a run.

Any issue with time traveling seems unrelated to this initial report. And I would use clockwork tests exclusively to validate the correctness/accuracy of your clock file. I wouldn't additionally perform any functionality testing in there.

I'm going to close this issue in favor of tracking with #1.

ce07c3 commented 6 years ago

It's related in the sense that Clockwork::Test alters core libraries such as Time leading to additional failures because there are side effects, e.g. clock skew. I am not testing additional functionality in our clock tests. It's hard to reproduce because it depends on the order of tests and what tests also depend on an accurate time measurement, but for now, I'd say that there probably need to be hooks in the original gem rather than monkeypatching it like this.