Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Adds asset servers for save_and_open #609

Closed
wants to merge 1 commit into from

6 participants

@nyarly

Adds Capybara.asset_servers: is a hash of url => [path prefixes] which will be written analogously to directories in asset_root. capybara/rails is extended with a sensible default.

Upshot is: use save_and_open_page in a run, run 'rails server', and asset pipeline resources will get served by localhost:3000.

@joliss
Collaborator

Your patch doesn't apply cleanly on master. You should git rebase master, fix the conflicts, and then push -f.

@joliss
Collaborator

You could also squash the two commits into one, using git rebase --interactive, by the way.

@nyarly

Okay, I believe I've accomplished that rebase and squash. Specs pass.

@nyarly

Any work on retrying this pull?

@joliss
Collaborator

I've been meaning to try but haven't gotten around to it so far. Assuming it works the way I understand it from reading your text and the code, I'm conflicted: On the one hand, it solves a bothersome problem. On the other hand, it's a pretty ugly hack (solving an admittedly ugly problem). And I suspect it will cause funny things to happen when you start a JS app with a prepopulated DOM.

So I'm on the fence.

@nyarly

I'll admit, I wish you'd been more explicit about your concerns when I suggested the change on the mailing list.

My reasoning is that, with the Rails 3.1 asset pipeline, there's no other reasonable way to get assets injected into save_and_open pages. The alternative is to precompile assets in dev env, but that completely bollixes the normal dev env server as soon as any assets change. (There was another problem with precompiling in development, but it's subtle and I don't remember what it is until it bites me.)

Part of the reason that I put in the extra time on making the addition configurable is that, on the one hand, different devs might have different development servers set up (e.g. Apache local servers instead of 'rails server'), but I don't see why if there was an issue with a prepopulated DOM, the solution couldn't be to configure Capybara.asset_server = {} to disable it.

@joliss
Collaborator

I'll admit, I wish you'd been more explicit about your concerns when I suggested the change on the mailing list.

Right. Sorry about that.

... there's no other reasonable way to get assets injected into save_and_open pages.

Hm. nods

I don't see why if there was an issue with a prepopulated DOM, the solution couldn't be to configure Capybara.asset_server = {} to disable it.

Sure. Perhaps my issue stems from the fact that I mostly test single-page apps, where I think this change is actually making things more confusing (and by default so). E.g. in my Backbone app here, if I save_and_open_page, it's now styled alright, but the JS app instantly reinitializes and overwrites the app div, essentially resetting the state to what it was at load time. So if say an element was renamed, it visually appears reset to the original state, until you View Source and discover that the DOM has indeed changed.

I can see that it's an improvement for multi-page apps though.

Anyways. These are just my (confused, rambling ^^) thoughts. I think Jonas has better judgement on this. Let's see what he has to say.

P.S. I just tested it in my Rails app and it appears to be working fine.

@nyarly

Seems like the simplest thing in the world to change the default to {} and let capybara/rails.rb set up a sensible default.

@joliss
Collaborator

Oh, that's a single-page Rails app I was talking about, actually. :) Defaulting to {} (or nil) and setting it up in rails.rb is exactly what your patch is doing (and rightly so), isn't it?

@nyarly
@nyarly

So, next thought: how general a case is single-page Rails? Because I'm the "use apache for dev" guy, which I didn't consider the common case, either.

And: are you using precompiled assets, or what? How do your saved pages working without this?

@joliss
Collaborator

I don't know how common it is, actually. Re assets, I'm not precompiling in dev/test mode. With your patch applied it just pulls the live-compiled JS files from the dev server. (Without it applied, I see a black-and-white page showing the DOM state, and no JS is working obviously. The black-and-white rendering is pretty useless too, though perhaps less confusing for newbies.)

@nyarly

Any word from Jonas on this? I'd like to stop having to monkey patch in my apps :)

@joliss
Collaborator

Let's cc him: @jnicklas, care to take a look?

@jnicklas
Owner

My suggestion is this: yank the whole asset rewriting completely.

1) It's never quite worked
2) There are better ways of debugging Capybara tests
3) It's such a horrible hack

Thoughts?

@nyarly
@joliss
Collaborator

I'd be interested to hear about the better ways

I like to use binding.pry, so I get a Ruby shell and I can also interact with the browser.

having to use Firebug would be unfortunate

I assume once you've paused the test with binding.pry, pasting the current URL into a Chrome browser should work too.

@nyarly
@kkelly18

I applied the diff to my installed gem in a Rails 3.2 environment. It didn't appear to work at first, but after starting Rails Server it did. Now running rspec with Rails S running or not, I seeing my assets displayed as I expect.

Would enjoy reading ptrs to other techniques for debugging.

@nyarly

Kevin, are you running a local Rails server for development? This patch does the same sort of rewriting that save_and_open does, but adds 127.0.0.1:3000 (by default) for URLs whose path starts "assets" (by default).

So: you might not be running 'rails server' on port 3000. Or there might be a problem - check the source of the files that result and confirm that assets paths are rewritten?

@kkelly18
@nyarly

Any chance this'll be merged? @jnicklas ? Or are the save_and_open just going away?

@EricR

What's the status on this? I can't get js or css to load on my Rails 3.2.3 app. I have a feeling this is related to this issue? I get 404 errors when it tries to fetch the resources.

@jnicklas
Owner

I am making an executive decision here by virtue of my status as Benevolent Dictator for Life of Capybara:

Asset rewriting has got to go. It is seriously the worst part of the entire Capybara code base. It's a terrible hack which has never worked right in the first place and the slim benefit it gives us can easily be achieved by superior debugging methods, like running the same test in Selenium and halting execution via e.g. sleep, gets or binding.pry.

I'm sorry if this annoys some of you, but it's my job to make the tough decisions about what goes in and what stays out. If I don't do that job and simply accept everything, we will not end up with a good result. So I hope that you can accept this decision even though you may disagree with it.

@jnicklas jnicklas closed this
@sdhull

FWIW, @jnicklas I think you should care more about how people need to use Capybara than having Capybara's code itself look good.

I've only just gotten capybara-firebug working, but prior to today I literally had no feasible way to debug js issues in my request (now "feature") specs. And even at that, my workflow sucks for debugging js issues (run specs, edit failing tests to change them to :driver => :selenium_with_firebug and add a debugger in each failing example, re-run failing examples & poke around in firebug).

Compare this with simply accepting this PR, which would enable developers -- regardless of whether they are using Selenium or not -- to actually be able to debug their js. I'm mainly discussing WRT to rails devs, as it's my opinion that the asset pipeline causes most of the headaches.

At the end of the day, the 20+ developers in my office would have benefitted from this functionality, as would (I'm sure) many many others. There are enough stackoverflow questions and at least 2 issues (if you count this one) on the Capybara repo itself about issues with assets and save_and_open_page. Surely you've heard about the user researcher at Yahoo! that found there were 50,000 people who experienced a given problem for every ONE that complained about it...

Just my 2c. I guess we'll continue working around the limitations of the tool.

@jnicklas
Owner

@sdhull you're right, I do care a lot about how the code looks. Fiercely. But you know what, for a tool as widely used as this one, we have surprisingly few actual bugs reported. This is a library that people use to write tests, which means we have a responsibility not to fuck shit up, so I'm not going to apologize for the fact that I am pedantic and that I throw out stuff which is crap.

You know save_and_open_page still works, right? You can open the page in a browser and take a look around, browse the source, etc… If you add CSS and JS and you're using RackTest, you're making it worse, because RackTest doesn't have CSS and JS, so now you're seeing the page differently from how Capybara would. How is that better?

How would completely altering all paths to all linked files help people debug stuff which is broken in the asset pipeline. Under RackTest, assets aren't even loaded, what possible effect could they even have on why your tests are breaking?

(Also, look into Pry, it's the bomb for all kinds of debugging)

@sdhull

Sure, that's accurate for those only using capybara to test with with rack test. Perhaps you should remove support for other drivers? :trollface:

Fact of the matter is, websites do depend CSS & JS. The webkit driver (or similar) is much more useful for writing specs that will test real user experiences. Most of our specs are written with the webkit gem, as most of our user interactions depend on javascript.

So. When capybara can't find content on the page that "clearly shows up on the page!", it is actually helpful when you save_and_open_page and CSS renders and you see "oh. that content is hidden with css".

And when my js-dependent specs don't pass, it would be nice to be able to poke around in webkit developer tools javascript console and see "Oh, my js didn't execute because of this js exception." Or whatever the case may be.

I would guess that more than 1/2 of developers using Capybara use it with a driver that allows them to test javascript. Of course, that's just a guess.

All that said, please don't misunderstand. I love capybara and believe that it's a great tool. It's just caused me a lot of pain in the past couple of months and I can't just continue plugging along anymore without at least voicing my opinions. Also if you were to actually remove support for drivers other than RackTest, every project I've ever worked on that used Capybara would immediately stop using it and find something that did support drivers that can execute js. I'm just one person but I've worked at quite a few different startups here in SF.

I like Pry, though I don't know as much about it as I'd like to. Can it help me debug javascript on my pages when it only seems to be failing in specs (eg, non-reproducible in dev or staging environments)?

@jnicklas
Owner

Hmm, I misread your comment.

Of course we aren't going to remove support for other drivers. That's the reason I wrote Capybara in the first place, after all.

I always use Selenium with Chrome, instead of Firefox, for debugging purposes, since its built in developer tools are far superior, it's a lot easier to debug. No messing around with Firebug.

As for using Pry, just add binding.pry right before whatever your failure is, and then use find, all, html, find("foo").visible?, find("foo").text or whatever else gets you closer to figuring out why your test is failing. If that doesn't work, I would switch to Selenium and take a look at it in a real browser.

If you're dissatisfied with the state of the project, then please, please help us out. We're just two people running this show, and it's a lot of work. But don't come here and threaten and accuse and point fingers, because that won't help anyone in any way.

@sdhull

I apologize for comments that came off as threatening/accusatory. Yesterday was the culmination of months of frustration with Capybara & the asset pipeline. I certainly did not intend to be threatening or accusatory, and I think that my frustration translated poorly in my comments.

Aha! Didn't realize I could use Selenium with Chrome! That will be a great improvement; I also prefer it's developer tools over Firebug.

Pry is cool, but I don't think it will help me debug broken javascript. In your example of usage, there's nothing really to set it apart from a debugger.

As for contributing, it is extremely disheartening to see this PR. This PR, as well as #485, #373 as well as various discussions on the web seem to indicate that developers want to use save_and_open_page for debugging that goes beyond simple content & markup.

Someone even went so far as to submit a PR to make it work and it was unceremoniously rejected. This does not inspire me to submit my own PRs. This pretty much makes me despair.

@sdhull

The reason save_and_open_page is better for debugging than going full-blown Selenium + debugger or pry is that in most cases I simply want to do a post-mortem on failing specs.

So in my spec_helper.rb I have:

config.after(:each) do
  DatabaseCleaner.clean
  save_and_open_page if example.exception && page.current_path.present?
end

And I can run my specs, grab some coffee while they run, and if something fails -- whether it's content, markup, CSS, or JS that went awry -- I can come back and examine the pages and the error messages that were left for me.

If something has truly gone bad, then a more interactive debugging session with Selenium (driving Chrome) will be excellent.

@jnicklas
Owner

@sdhull if you can come up with something that isn't as much of a hack as this is, then I'd be okay with accepting a pull request like that. My issue with this pull request isn't what it's trying to fix, My personal opinion aside, if you can come up with a solution which gives a better debug experience then that's great, and I'd be happy to merge it, but it needs to be not terrible.

Some pull requests get rejected, that's the natural order of things. As maintainer, I have to make decisions, I can't just blindly merge everything. If you send me something I promise I'll consider it, but I can't promise I'll merge it, that's just a risk you have to take.

@sdhull

Fair enough.

Just to be clear, what specifically did you take issue with? The implementation of #rewrite_css_and_image_references or the fact that it exists?

@jnicklas
Owner

One huge no-no about this pull request was that it made the assumption that there is a development server running at localhost:3000. Ideally we would make no assumption about any other server processers being started. If we need a server running, we should start it ourselves, or use the existing Capybara::Server somehow.

Also, we should use a proper HTML parser in order to inject the new asset paths, instead of relying on regexps. Parsing HTML with regexps is stupid.

@sdhull

So... if I were to revisit this PR and have it make no assumptions about dev servers and to use a proper HTML parser in order to inject new asset paths, you would reconsider this?

@jnicklas
Owner

Yes.

@sdhull

Took awhile for me to get around to it, but finally got something to show: #958

Again -- apologies for my outrage. Capybara is like miles better than Webrat and I'm very happy it exists. :wink:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Jan 12, 2012
  1. @nyarly
This page is out of date. Refresh to see the latest.
View
6 lib/capybara.rb
@@ -17,6 +17,7 @@ class InfiniteRedirectError < TimeoutError; end
class << self
attr_accessor :asset_root, :app_host, :run_server, :default_host
+ attr_accessor :asset_servers
attr_accessor :server_host, :server_port
attr_accessor :default_selector, :default_wait_time, :ignore_hidden_elements, :prefer_visible_elements
attr_accessor :save_and_open_page_path, :automatic_reload
@@ -35,7 +36,10 @@ class << self
# === Configurable options
#
# [asset_root = String] Where static assets are located, used by save_and_open_page
- # [app_host = String] The default host to use when giving a relative URL to visit
+ # [asset_servers = Hash] Where dynamic assets are hosted -
+ # hash of URL => path prefix
+ # [app_host = String] The default host to use when giving a
+ # relative URL to visit
# [run_server = Boolean] Whether to start a Rack server for the given Rack app (Default: true)
# [default_selector = :css/:xpath] Methods which take a selector use the given type by default (Default: CSS)
# [default_wait_time = Integer] The number of seconds to wait for asynchronous processes to finish (Default: 2)
View
6 lib/capybara/rails.rb
@@ -4,7 +4,7 @@
Capybara.app = Rack::Builder.new do
map "/" do
if Rails.version.to_f >= 3.0
- run Rails.application
+ run Rails.application
else # Rails 2
use Rails::Rack::Static
run ActionController::Dispatcher.new
@@ -13,5 +13,7 @@
end.to_app
Capybara.asset_root = Rails.root.join('public')
+if Rails.version.to_f >= 3.0
+ Capybara.asset_servers = {"http://127.0.0.1:3000" => ['assets']}
+end
Capybara.save_and_open_page_path = Rails.root.join('tmp/capybara')
-
View
8 lib/capybara/util/save_and_open_page.rb
@@ -32,7 +32,13 @@ def open_in_browser(path) # :nodoc
def rewrite_css_and_image_references(response_html) # :nodoc:
root = Capybara.asset_root
- return response_html unless root
+ asset_servers = Capybara.asset_servers
+ return response_html unless (root or asset_servers)
+
+ (asset_servers || {}).each_pair do |asset_server, server_assets|
+ response_html.gsub!(/("|')\/(#{server_assets.join('|')})/, '\1' + asset_server.to_s + '/\2')
+ end
+
directories = Dir.new(root).entries.select { |name|
(root+name).directory? and not name.to_s =~ /^\./
}
View
17 spec/save_and_open_page_spec.rb
@@ -143,6 +143,23 @@ def test_with_directories(directories)
it "should rewrite relative paths to absolute local paths" do
test_with_directories([ 'javascripts', 'images' ])
end
+
+ context "asset_server contains a URL and path" do
+ it "should rewrite relative paths to FQ URLs and/or absolute local paths" do
+
+ mock_asset_root_with(['javascripts', 'images'])
+ asset_server = "http://assets.com/"
+ Capybara.should_receive(:asset_servers).and_return(asset_server => ['javascripts'])
+ @temp_file.should_receive(:write) do |html|
+ html.should_not =~ %r{["']/?javascripts}
+ html.should_not =~ %r{["']/?images}
+ html.should_not =~ %r{#{@asset_root_dir}/javascripts}
+ html.should =~ %r{#{asset_server}/javascripts}
+ html.should =~ %r{#{@asset_root_dir}/images}
+ end
+ Capybara.save_page @html, 'page.html'
+ end
+ end
end
context "asset_root path contains no directories" do
Something went wrong with that request. Please try again.