New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add #travel
and #travel_to
to ActiveSupport::TestCase
#12824
Conversation
|
||
if block_given? | ||
block.call | ||
Time.unstub :now |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should not this be inside an ensure
block?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't matter; If the exception got raised, then the test case will fail and Mocha will handles the unstubbing for us anyway. (Note that in the normal case, I don't even bother unstubbing it at all).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True.
* Add `ActiveSupport::Testing::TimeHelpers#travel` and `#travel_to`. These methods change current | ||
time to the given time or time difference by stubbing `Time.now` and `Date.today` to return the | ||
time or date after the difference calculation, or the time or date that got passed into the | ||
method respectively. These methods also accepts a block, which will return current time back to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also accept
@sikachu did an edit pass, thanks! The examples seem unnecessarily complicated to me. The obvious example to depict this feature would be a bare invocation of (Note: I suggest specifically |
@fxn so it's ok if I mention I think I see what you mean, do you think something like this would be good? Time.current # => Sat, 09 Nov 2013 15:34:49 EST -05:00
travel 1.day
Time.current # => Sun, 10 Nov 2013 15:34:49 EST -05:00
Date.current # => Sun, 10 Nov 2013
travel 1.day do
User.create.created_at #=> 1 day from now
end |
Exactly, the changelog is fine because that's what the patch does, but I believe Those new examples look very natural to me |
@fxn, PR is updated with new example. Can you give it another look? |
Good, getting close. There is one remaining occurrence of "#=>" without space. Then, docs use "# =>" only to display actual value expressions. In the case of User.create.created_at #=> 1 day from now we'd use a regular Ruby comment instead: User.create.created_at # 1 day from now |
@fxn I just rebased. For some reason, I don't think you saw the right commit as I removed '1 day from now' and fixed the one last |
Yeah! For some reason I didn't see the new edits. We are close, let me do a final remark. Examples should run as they are shown, and if I am not mistaken the blocks should move another day, because A way to solve this in a way that flows is to introduce the features step by step, inserting a paragraph. Something like this:
See what I mean? That reads naturally, and the information is given one step at a time, easy to digest. I added an additional call to What do you think? |
I like that. Will make an edit and ping you again @fxn. |
Add `ActiveSupport::Testing::TimeHelpers#travel` and `#travel_to`. These methods change current time to the given time or time difference by stubbing `Time.now` and `Date.today` to return the time or date after the difference calculation, or the time or date that got passed into the method respectively. These methods also accept a block, which will return current time back to its original state at the end of the block. Example for `#travel`: Time.now # => 2013-11-09 15:34:49 -05:00 travel 1.day Time.now # => 2013-11-10 15:34:49 -05:00 Date.today # => Sun, 10 Nov 2013 Example for `#travel_to`: Time.now # => 2013-11-09 15:34:49 -05:00 travel_to Time.new(2004, 11, 24, 01, 04, 44) Time.now # => 2004-11-24 01:04:44 -05:00 Date.today # => Wed, 24 Nov 2004 Both of these methods also accept a block, which will return the current time back to its original state at the end of the block: Time.now # => 2013-11-09 15:34:49 -05:00 travel 1.day do User.create.created_at # => Sun, 10 Nov 2013 15:34:49 EST -05:00 end travel_to Time.new(2004, 11, 24, 01, 04, 44) do User.create.created_at # => Wed, 24 Nov 2004 01:04:44 EST -05:00 end Time.now # => 2013-11-09 15:34:49 -05:00 This module is included in `ActiveSupport::TestCase` automatically.
@fxn doc updated again, I added a paragraph (well, copied what you've typed here) and updated both changelog and the example. Would you mind give it another look? :D |
Thanks Prem! |
Add `#travel` and `#travel_to` to `ActiveSupport::TestCase`
I saw this in the CHANGELOG and I was curious why this was added when something like Timecop and Delorean work just fine and don't add even more code to ActiveSupport? |
We add simplified or desired versions of tech available in gems all the time. In fact, that's the preferred development path for Rails. Prove the concept in a gem, pull into core if it's a good fit. Expect this pattern to continue. Minimizing the amount of code in AS just on principle is not an objective we are optimizing for.
|
@dhh but adding to the code of AS unnecessarily shouldn't be either. Adding one line to a Gemfile is much better than having extra code which must be maintained for no discernable benefit other than "now I don't have to include this gem". Taking code from a gem into a larger codebase is defeating the point of modularity. And modularity is exactly the reason why you can use gems to test functionality in Rails 3/4. In Rails 2 you couldn't do that. You had to resort to hacks. If the time wasn't taken to modularize in Rails 3, we'd still be resorting to Rails plugin madness. So to say "modularity gave us all these benefits and we can now test functionality in gems first, but modularity isn't good enough that we should leave things in gems" seems like a fairly strange point of view. |
Guess we'll have to agree to disagree.
|
Aside from bloat factor this locks in users to Mocha style stubbing framework rendering those helpers useless for Rspec(-mocks) users :( |
The following test case is also failing for me with 4.1.0.beta1
|
Keeping a core library small is not just a principle for principles sake. It also makes for faster load time and tests, and enlarges the competitive landscape so that different tech can compete fairly. Also why add to the surface area of Rails which already does so much? It creates more vectors for bugs and support which is easily outsourced to external gems right now. If Rails does not make core lib size and speed a priority the same thing could happen as in the Java community where its common to boot overly complex application servers chock full of functionality for the simplest of things. |
Please redirect the conversation of "how big I think Rails should be" somewhere else. It doesn't add anything to this PR. |
Monkey patching arbitrary class methods on the Time and Date objects on the fly, is just ugly, IMHO. Ruby gives you enough rope to hang yourself with here. At worst, this functionality should ONLY be necessary in an integration test with a lot of moving parts, any of which could be calling Time.now or its variants. At best, when you are unit testing your objects in isolation, you should be able to inject dependencies like Time or Date on initialization so that you can easily hand the objects under test, mocks instead. This also prevents the need to alter class methods and other global changes such as that, adding concurrency stability. Remember, processors aren't getting much faster, so the only way to scale your growing test suite up is out, which means parallel test runs that don't clobber each other with global changes. |
So you'd implement, def age(with_respect_to)
...
end Yeah, in integration tests you could also inject date current via the query string in test mode wouldn't you? Because OMG mocking Some people prefer not to do that. I define query strings according to the application interface, not after integration tests. And I apply the same principle to the I don't say that to convince you of anything, I say that to tell you that there are different points of view on this matter and there are people that mock |
Xavier, On Feb 19, 2014, at 9:53, Xavier Noria notifications@github.com wrote:
|
I edited my example above to remove the default, because if |
Your test should be the first client of your code, so this literally makes no sense, tastes notwithstanding. |
That’s not how I program. Tests are important, but certainly not the only consideration in how you design your code. For me it starts with the feel of the API. Not just what’s easy to test. If you can’t make sense of people having a different approach to programming than the one you prefer, then I don’t think we’re going to make much headway here. On Feb 19, 2014, at 5:21 PM, Peter Marreck notifications@github.com wrote:
|
Tests are not client code in my view of things. Client code is client code, uses my code to build stuff. In a library I provide an API for others to use, either 3rd party or internal. Tests verify that my code does what is supposed to do. |
Anyway, what David said, there doesn't seem to be a lot more to discuss here regarding this particular patch. |
I totally respect that others have other styles. It's fine. I speak as a person who's worked on gigantic Rails codebases (I mean, seriously large, should-have-been-broken-out-ages-ago codebases that I'm unfortunately not in charge of), that's all, and you are welcome to disagree. Based on my experiences chasing down bugs and implementing features in said monolithic codebases, I now feel that dependencies should be visible (and painful), that test code is actually an excellent code design aid if you follow some kind of TDD, and that most tests in a suite should be unit tests which can test an object in isolation and which can run in parallel without clobbering other global states. As an example of the size of codebase I'm used to dealing with, our Rails stack takes 40 seconds to load, our test suite takes > 40 minutes to run (and is mostly integration tests, or at least, not "true" unit tests), and here's how big our Gemfile is: cat Gemfile | wc -l #=> 220 I apologize for turning a helpful PR into a diatribe. I am a member of a team that is sore from dealing with extensive Ruby/Rails technical debt, and these are just my observations. You guys go and kick butt, just trying to shed some light based on my large-codebase experiences. ;) |
Peter, that sounds rough! And I can certainly see how you can draw different conclusions on what good design entails when you’re faced with disentangling such a ball of mud. Just like I draw conclusions on good design from working with a stable team of developers who know their shit, and a 30KLOC Basecamp code base. It’s certainly easier for me to remain optimistic than you, I’ll grant you that ;). But as a general approach, Rails follows a design approach where we set high expectations, and hope people will follow us there. It’s a collection of sharp knives with great grips. It’s easy to hold, comfortable to use it correctly, but also absolutely possible to cut your toes off. On Feb 19, 2014, at 5:43 PM, Peter Marreck notifications@github.com wrote:
|
@fxn having In general I agree with @pmarreck on the issue of making dependencies more explicit. If you have so many of them that it bothers you when they are explicit maybe you need to rethink your design. |
@orend how do you test the age of the user shown in his profile page in integration tests without mocking?
The current time and the current date are global state. I believe mocking them is perfectly valid. But this is not the place to continue this discussion, this is a PR for a particular patch and it is clear that we believe the feature implemented here is legit. |
The explicitly-visible-dependency thing is important (IMHO) because exposing it exposes the modularity (or lack thereof) of your code. If you have to pass in 10 params to your method just so that it doesn't have to call out to anything else it wasn't handed explicitly, the "ugliness" there is actually the 10 external dependencies, not the 10-parameter argument list. If you ever need to refactor that guy, it's going to be hard to untangle it, move it elsewhere, etc. without breaking a lot of things. A lack of modularity results in exactly the "ball of mud" @dhh was referring to... IF you don't have a tiny little Rails app. Basically, you can have your large Ruby codebase, but it will turn into a slowly encroaching hell without modularity... The mud will turn into concrete. Injection exposes lack of modularity explicitly. It's debatable of course (as is all design, which is great!) I have a crazy faith that large Ruby (which would include Rails) codebases are not only possible, but maintainable. I also know that a lot of people are NOT with me, there. ;) Granted, I think what @orend and I are referring to has been informed/influenced by some functional programming reading material (as well as ideas such as PORO http://rubyreflector.com/Plain-Old-Ruby-Object) as well as by being affected by working within the complexity of a large codebase. Things like forking (whether it's by Unicorn in production, by Zeus in development or by parallel_tests in a test suite), when combined with changing global things, DON'T seem to mix well :O Time is kind of a unique case as it's dependent on something in the environment (the clock chip output). Agreed to table discussion here since this is a PR but sometimes things occur organically, and it wouldn't be the first time a crazy github discussion occurred on something seemingly unrelated :) |
@fxn and whoever is interested - we can continue the discussion here https://gist.github.com/orend/9099893 |
Let me put it another way: This particular patch is not a generic declaration of intentions that says that not passing dependencies is good. That's something out of the scope of this patch. And I am sure we would generally agree more than disagree on a case by case basis. The scope of this PR is just these helpers, we believe they have valid use cases, and are going to ship with 4.1. |
@pmarreck but you cannot discuss in general whether explicit is better than implicit. That's a too vague topic. I can discuss about concrete use cases on the merit of the solutions for those use cases. In the case of |
@fxn Regarding User.age, what I'd likely do in this case (at this point in my career) is write an initialize method on User which first calls super using only the attributes that ActiveRecord expects and then sets additional dependencies based on whatever else is passed in. In your test it would look like created_time = Time.at(some_deterministic_time)
fake_now = created_time + 5.seconds
subject_under_test = User.new(created_at: created_time, now: fake_now)
assert_equal 5.seconds, subject_under_test.age User, in turn, will use any passed-in "now" parameter in place of Time.now in its code, defaulting to Time.now. So any "time" computation would therefore use passed-in values in a unit test, and default values elsewhere. You avoid having to use procedural code which modifies/patches global state such as "travel 1.day", and you avoid having to use special tools to make that happen. And you don't even have to have every one of your methods accept dependency params, just your initializer. A lot of this design principle is difficult in Rails because of the way Rails was designed... it tends to go a bit against that grain. And most of this was designed BEFORE it started to look like some of the decisions might have negative repercussions in a very large codebase. :) Some thought here also influenced by Gary Bernhardt's excellent https://www.destroyallsoftware.com/talks/boundaries which I was very intrigued by. |
Oh well, if you want to keep it here… @fxn - to answer your question: I would test that the age of the user is shown in his profile page but not by asserting the exact age. I think this will be over-testing. In the collaborators' unit test you'd have set expectations that the |
@pmarreck I see what you mean, and we are not going to agree because our inner drivers are different. You'd inject a "now" because you believe mocking Time is bad and you feel more comfortable passing a "now" in unit tests. That's fine. I wouldn't because I don't want users of the class (including me myself) to be wondering in which case are they supposed to pass a "now". Answer: never. We put our emphasis on different places. It is fine. |
I had a whole response written and decided against posting it since it would have done absolutely nothing to change anyone's minds and I hope @pmarreck and @orend will follow suit. Honestly I wish I'd never even made the first comment to kick all this off. @dhh I did have quick question. Very roughly do you have any idea of how much of Basecamp Classic's codebase was able to be reused in Basecamp Next? I won't reply, I'm going to let this thread die. As @fxn said, this is getting merged in regardless and the merits of DI was not the point that I was trying to make in my first comment anyway. |
very interesting read guys! please don't forget, that most of us love rails BECAUSE of it's opinionated way of doing things. |
Add
ActiveSupport::Testing::TimeHelpers#travel
and#travel_to
. These methods change current time to the given time or time difference by stubbingTime.now
andDate.today
to return the time or date after the difference calculation, or the time or date that got passed into the method respectively. These methods also accept a block, which will return current time back to its original state at the end of the block.This module is included in
ActiveSupport::TestCase
automatically./cc @dhh, and please do grammar check for the doc for me, @fxn.
Also, I was about to add a guide section for this, but I couldn't find a good place to add it. Any suggestions are welcomed!