Skip to content
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

Update test names and locations #7878

Merged
merged 1 commit into from
Oct 10, 2012
Merged

Conversation

blowmage
Copy link
Contributor

@blowmage blowmage commented Oct 8, 2012

As discussed on the rubyonrails-core mailing list, this request changes the default test locations as follows:

test/units          -> test/models
test/units/helpers  -> test/helpers
test/functional     -> test/controllers
test/functional     -> test/mailers
test/integration    -> test/acceptance

The existing rake tasks are backwards compatible, and will run tests in the old and new locations. New rake tasks were added that only runs the tests in the new locations. The new tasks are test:models, test:helpers, test:controllers, test:mailers, and test:acceptance.

attn: @spastorino and @tenderlove

@steveklabnik
Copy link
Member

:shipit:

Looks good. Thanks for this work. ❤️

@xenda
Copy link
Contributor

xenda commented Oct 9, 2012

:shipit:
This is a move made with pure love.

@jeremy
Copy link
Member

jeremy commented Oct 9, 2012

@blowmage could you squash the commits?

Not sure about changing integration -> acceptance. That one doesn't seem necessary.

@steveklabnik
Copy link
Member

FWIW, rspec calls them 'request' specs, since they test a request.

@xenda
Copy link
Contributor

xenda commented Oct 9, 2012

👍 to requests, if it's open to discussion.

@blowmage
Copy link
Contributor Author

blowmage commented Oct 9, 2012

@jeremy Yes, I can squash if/when given a 👍 for accepting the pull request.

I don't think the name "integration" is appropriate is going on in the integration tests. The tests exercise the whole stack, from the router/dispatcher to the controller, views, models, etc. The tests can have one session with multiple requests. Both "integration" and "request" are too narrow IMO. I would love a discussion on a better term.

http://c2.com/cgi/wiki?AcceptanceTest
http://c2.com/cgi/wiki?IntegrationTest

@stevenharman
Copy link
Contributor

Rspec calls them 'request' or 'acceptance' tests, depending on how you roll.

My tendency is to go with spec/acceptance and to limit those to true acceptance tests - driven from the outside, from the perspective of the consumer, ignorant of as many implementation details as possible.

@rafaelfranca
Copy link
Member

@steveklabnik
Copy link
Member

Seems legit.

@coreyhaines
Copy link
Contributor

+1 - will be nice to see the old confusion-causing names drift away

@josevalim
Copy link
Contributor

Strong 👎 for test/acceptance . They are definitely not acceptance tests.
test/integration may be too restrict (I don't think so), but
test/acceptance is plain wrong.

José Valim
www.plataformatec.com.br
Skype: jv.ptec
Founder and Lead Developer

@zires
Copy link
Contributor

zires commented Oct 9, 2012

Looks more clear. 👍

@jeremy
Copy link
Member

jeremy commented Oct 9, 2012

Integration tests are really HTTP resource tests. They're called "integration" to indicate that they exercise the whole app stack.

I'd consider calling these Resource tests instead of Integration tests, in keeping with the other changes from "how we're testing this" to "what we're testing."

@steveklabnik
Copy link
Member

Uuuuuuuuugh. I really, really don't want to bikeshed this, but Rails messes up the REST terminology so bad already, can we just stay away from that?

@garybernhardt
Copy link

+1 on everything except acceptance; -1000 on that. Call it "integration", "system", "request", whatever. Just don't call it "acceptance". Acceptance is not a test scope; it's a test objective. An acceptance test can be isolated to ten lines of code or integrating across 10,000.

@steveklabnik
Copy link
Member

After some discussion, I agree with @jeremy. 'resource' seems like a reasonable name.

@koffeinfrei
Copy link

I agree with @garybernhardt that acceptance is not a test scope, though in most cases acceptance tests are functional tests. They differ from integration tests in that they are black box tests which test the application as a whole, disregarding any knowledge of the internals. Integration tests are basically unit tests with an external dependency, e.g. model tests.

Since functional is already taken as a legacy term I favor system tests.
That said, I'd still prefer acceptance over integration though.

@dvrensk
Copy link
Contributor

dvrensk commented Oct 9, 2012

+1 on request tests. Acceptance testing has been the prevalent test type long before automated testing was known to more than 1% of developers, and it is still something that most enterprise projects can not be done without manual testing. Taking a term with a long history and slapping it onto something that fits in the framework at hand is exactly the mistake that we are trying to fix by renaming 'unit tests' to 'model tests'. Please don't repeat that mistake.

(I know that I make exact and unsubstantiated claims above, but I hope that you agree with the reasoning anyway.)

@blowmage
Copy link
Contributor Author

blowmage commented Oct 9, 2012

Here are my thoughts. I agree with @koffeinfrei's definition of the term "functional" and to my thinking it is the most appropriate one to use, but it would cause confusion since the term has already been used by rails and I was asked to maintain backwards compatibility in this change. It looks like "request" is the most popular term but I think it is too narrow; many of these tests involve multiple HTTP requests. Also, it appears to have been deprecated in RSpec. I am intrigued by the term "resource", but again they can exercise several REST-ish resources in a single test and I think that could cause confusion. That leaves "system", which describes the right scope for these tests, but I have usually thought of system tests as QA tests and not developer tests.

In summary, here are the terms discussed so far in order of my personal preference and considering the trade-offs I shared above:

  • system
  • acceptance
  • resource
  • request
  • functional
  • integration

@stevenharman
Copy link
Contributor

Given the options, and all of the feedback, I too am inclined toward acceptance. However, I realize I may be biased and have a personal style which lends itself to calling them acceptance.

[My tendency is to] limit those to true acceptance tests - driven from the outside, from the
perspective of the consumer, ignorant of as many implementation details as possible.

The most compelling reasons I've heard against acceptance is @garybernhardt's objection that acceptance is an objective not a scope. While true in part, it has also long been used as a label for a style of testing (see: http://c2.com/cgi/wiki?AcceptanceTest).

Perhaps there is no great term that is free of preconception and/or prior definition?

@blowmage
Copy link
Contributor Author

blowmage commented Oct 9, 2012

FWIW, the reason "acceptance" wasn't top of my list was because of the objections of some core members. I agree with @stevenharman's comment since that is how I use these tests and why I originally chose the term. I would be perfectly happy if it was kept.

@justinko
Copy link
Contributor

justinko commented Oct 9, 2012

requests please.

Also, please add test:lib.

@jasonroelofs
Copy link

@justinko Given that Rails doesn't generate lib or set up an autoload path for it anymore, it probably shouldn't be added here.

I vote for acceptance, I'm familiar with that terminology.

@blowmage
Copy link
Contributor Author

blowmage commented Oct 9, 2012

@justinko I have an idea for a separate pull request that will address generating rake tasks for additional directories under test. So tests in test/lib and test/presenters and test/serializers and test/decorators would be run with:

rake test:lib
rake test:presenters
rake test:serializers
rake test:decorators

But that will be a separate pull request. :)

@justinko
Copy link
Contributor

justinko commented Oct 9, 2012

Who is accepting the acceptance tests?

@stevenharman
Copy link
Contributor

My workflow tends to look something like:

  1. write acceptance test in high-level prose, from the perspective of the consumer.
  2. when there is an embedded customer, review the test with them. Lacking that, I put on my customer hat and do my best to think like a consumer.
  3. start turning the test green by implementing the prose, outside-in. (For me this often means rspec driving capybara, with numbers of lower-level developer tests as I move down through my feedback loops.)
  4. when the outer test goes green, I'm done. Review the test again (with my customer hat on if necessary) to make sure my expectations are still met.

After that the test becomes an artifact and is mostly used to as a regression safety net and to remind myself and others of the intention and desired value of the feature.

All of that said, I fully understand that others don't work that way. It seems @blowmage and I may have similar styles which is why we are advocating, or at least not against the term acceptance. Perhaps others find the term too prescriptive?

@stevenharman
Copy link
Contributor

Question: is all of the contention around the name acceptance caused by feelings of it being too prescriptive? I can certainly understand if that's the issue - especially given what I've already said about it being a style to me.

Maybe folks are generally more comfortable with sticking to the scope at which tests interact with the system, as @garybernhardt has suggested, rather than they style in which they are written?

@garybernhardt
Copy link

The percentage of teams writing actual acceptance tests is not very large, as far as I can tell. I certainly don't think that the average Rails team is writing them. If we rename integration/functional/system tests to "acceptance" tests, we recreate exactly the problem that this PR is designed to solve. Instead of confusing Rails programmers about what "unit" means, we'll confuse them about what "acceptance" means.

@stevenharman
Copy link
Contributor

@garybernhardt Agreed. I suppose I've not had an adverse reaction due to my own experiences and process wherein I do write acceptance tests.

I'm coming around to the idea keeping with the "scope" of the tests, which while prescriptive, seems less so that any name which also presumes the style of the tests. Does that track?

@blowmage
Copy link
Contributor Author

blowmage commented Oct 9, 2012

I'm happy to update the pull request to change "acceptance" and squash the commits, but I feel like there isn't a consensus on a replacement term just yet. Here are the options as I see them:

  • system
  • requests
  • resources

How can we gain a consensus?

@garybernhardt
Copy link

I'm a fan of simple, established names when available. I call those things integration tests or system tests, and I don't really distinguish between those two types any more. Calling them "request" or "resource" tests doesn't afford communication with other people. If you talk to a J2EE or Django programmer about your resource tests you're going to get a blank stare. ;)

@dvrensk
Copy link
Contributor

dvrensk commented Oct 9, 2012

I take back my +1 for 'requests' since, as @blowmage points out, a single test might contain several requests. For the same reason, I think that 'resource' is a bad idea, but I am still interested in hearing what its proponents have to say.

Out of the three alternatives above, I prefer 'system', and I think it is a good name since the tests exercise the system that I'm building, albeit not necessarily the system of which this project might be a part. If we haven't reached the point of a formal vote, I would like to add another option: "story tests". They do, after all, test stories. But only if you work in stories, of course.

@justinko
Copy link
Contributor

justinko commented Oct 9, 2012

Ah, yes, for some reason my brain thought "integration" was taken. I change my vote from "requests" to "integration".

You are running a test that "integrates" parts of the system from the highest level available. Self explanatory.

@stevenharman
Copy link
Contributor

You can, of course, integrate at any level. Just hitting an ActiveRecord-backed object is integrating across system boundaries. And testing with any two real (non-stub/mock) objects is integrating across those objects' boundaries.

So... kinda' self explanatory. ;)

@justinko
Copy link
Contributor

justinko commented Oct 9, 2012

@stevenharman In the context of testing, "integration" tests hit the other "scopes" (model, controller, etc.). They "integrate" the lower level scopes.

@stevenharman
Copy link
Contributor

Sure. I was being pedantic/an arse. The point is, integration can also happen in those other "scopes," and a label that may be applied to many different categories of tests.

Naming is hard. Let's go shopping!

@jeremy
Copy link
Member

jeremy commented Oct 9, 2012

Historically, functional and integration tests are both controller tests. Their primary differences lie in their APIs, where they hook in to the Rack stack, and how much state they maintain.

Functional tests interact with a bare controller, unencumbered with middleware. You spec out a controller's behavior more intimately, but looking at what exceptions are raised and by which template is rendered. Pretty deep into internal details.

Integration tests interact with the Rack stack using an HTTP API: get '/messages/123' and assert_response :forbidden. The API is built to interact with your HTTP resources and spec out their behavior. They're hands-off when it comes to controller internals.

We'd hoped that the HTTP-oriented integration testing API would eventually take over from the functional testing API, but we haven't really done much to encourage that. The functional testing API works fine, it's easy to think in terms of controller actions rather than URLs, and having access to internals can be convenient. You can go full-on with the integration testing API; never write a functional test. It changes the way you think about what you're testing: the controller vs. the HTTP resource.

Pragmatically speaking, writing tests using the functional API reflects that you're testing the behavior of controller actions. Writing tests using the integration API reflects that you're testing the behavior of HTTP resources.

Barring deprecating and phasing out the functional API entirely, it's sensible to call functional tests "controller" tests and integration tests "resource" tests. We should change the resource generators to prefer resource tests over controller tests, too. Encourage thinking in terms of HTTP interaction rather than controller+action state.

@blowmage
Copy link
Contributor Author

blowmage commented Oct 9, 2012

Naming is hard, and time consuming. So I removed the commits relating to the "integration" changes so it won't hold up the changes most seem in favor of. I also squashed the remaining commits.

I still think "integration" is a poor name, and I would like to see it replaced, but we can paint that bike shed in a separate pull request. :)

@jeremy
Copy link
Member

jeremy commented Oct 9, 2012

Good choice, @blowmage!

@jeremy
Copy link
Member

jeremy commented Oct 9, 2012

One last thing: could you update the CHANGELOGs as well?

Change the default test locations to avoid confusion around the common
testing terms "unit" and "functional".
Add new rake tasks for the new locations, while maintaining backwards
compatibility with the old rake tasks.

New testing locations are as follows:

    app/models -> test/models (was test/units)
    app/helpers -> test/helpers (was test/units/helpers)
    app/controllers -> test/controllers (was test/functional)
    app/mailers -> test/mailers (was test/functional)
@blowmage
Copy link
Contributor Author

blowmage commented Oct 9, 2012

@jeremy CHANGELOGs are updated.

jeremy added a commit that referenced this pull request Oct 10, 2012
Update test names and locations
@jeremy jeremy merged commit 1c534c6 into rails:master Oct 10, 2012
@jeremy
Copy link
Member

jeremy commented Oct 10, 2012

Thanks for seeing this through, @blowmage! 🍻

@spastorino
Copy link
Contributor

@blowmage thanks man!

@steveklabnik
Copy link
Member

:shipit: 👍 ❤️ ❤️ ❤️

@neersighted
Copy link

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.