-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Added Rails 3.1 asset pipeline support #500
Conversation
Oh, btw, this relates to #485. |
Works great - thanks very much for this. Css displays perfectly, some pictures don't display as normal but it's more than adequate for save_and_open_page. |
@tracedwax, are you able to figure out why some images aren't displaying as normal? They should all work. Maybe check your manifest.yml file to make sure they're getting included in there, and then make sure the image |
to cofig.assets.manifest for path to manifest file.
Thanks for this! I get the following error (Ruby 1.8.7, Rails 3.1.1)
|
JangoSteve, plz replace in lib/capybara/util/save_and_open_page.rb:58 "Dir.exists?()" to ".File.directory?()" for compatibility with ruby 1.8.7 |
Hey @dom1nga good point about 1.8.7, I guess I've been using 1.9.2 for so long I forgot that was a new thing. I was wondering the same thing when I started working on a fix. Trust me, requiring an asset precompile was not my first choice. The issue is that, with the asset pipeline enabled, assets are now part of the app; javascripts, stylesheets, and images are no longer static files that bypass the rails stack, they actually go through the rails process and get interpreted at runtime just like views. The only way we could get them to render in the However, that would require the localhost app to still be running after our This was easy before, because all assets were just rendered as static files from the public directory anyway, so there was no difference if you loaded them from the dev server path or from a file page. And so The only other option would be to make Taking all fo these considerations into account, this was the best solution I could come up with. |
@jfi, if you open > ::Rails.application.config.assets.enabled
> ::Rails.application.config.assets.manifest The only time I know of that the latter would be |
@tracedwax, I've encountered a few times where certain images or stylesheets weren't being rendered properly, and it was because I had forgotten to actually add those files to the asset pipeline (meaning they weren't being rendered properly in production either, only I hadn't known that because they were fine in dev). |
Sorry to take so long to weigh in on this. I am never going to accept a patch which does anything Rails specific, even if you put it in a conditional. I honestly think this is a lot of overhead, for very little gain. If you desperately need this, temporarily switch to selenium instead. |
@jnicklas, am I correct in the reasoning for why |
Pretty much. Copying all the assets seems like overkill. I think we shouldn't overengineer this, there are better alternatives when this kind of precision is required. |
Agreed. That's why my patch here does not copy any assets.
Any suggestions? The problem is that, in Rails 3.1 and later, assets are no longer static files, they are compiled as a part of the asset pipeline. That means either: a) They are not precompiled, and thus, rely on the Sprockets middleware running with the rails server. b) They are compiled, in which case, the filenames are different, and the rewritten paths from Either way, it doesn't work. Capybara's solution to the page requiring the server to render was to copy the html over to a tmp directory. I didn't want to use that same solution for the assets that require the server, because that would require copying all the assets over too (and like you said, that would seem overkill). So, instead, this patch just rewrites all the asset paths to the precompiled asset paths, instead of blindly rewriting the paths to the There may be a way we could do this without requiring Rails-specific code, though. I'm going to try something else, and if it works, I'll open a new ticket. In the meantime, everyone who needs to use |
@JangoSteve +1 |
I just don't see this as a big issue, and I disagree that it needs fixing at all. Switch to selenium, use pry and you'll have a debug environment which is 100 times better than save_and_open_page. And even if you use it as is, you can still probably see most of the relevant markup just fine, even if CSS and JS are stripped. |
@jnicklas, better to open a page, than every time to think about whether enough seconds set for sleep. Why watch "cartoon" of, for example, twenty episodes, if you want to watch only twenty-first? Let's all give up the use of webkit. More cartoons! More selenium! |
@jnicklas, that's the point, you can't see most of the relevant markup just fine. Everyone on my team has stated that |
The fork from @JangoSteve has worked well for me and I will continue to use it if that is the best option. That being said, I would just as soon not see Capybara be fragmented into multiple branches if @JangoSteve is willing and able to work towards a solution compatible with the vision and constraints of the project. @jnicklas - Since this does not seem to be a core concern for you and already depends on launchy, are you thinking that save_and_open_page should be pulled out into a separate gem? |
So um, can everyone having problems with this try something? After spending half the day combing the source for sprockets and sprockets-rails, I think I may have found a simpler solution. Add this to config/environments/test.rb:
Then run:
By default And with that, capybara's default path rewriting will match the undigested filenames and things will work. If you want to get rid of all of these files when you're done debugging, you can run:
If you call Perhaps we could add something to the readme or something that tells people to run precompile in test mode before using |
Hey @JangoSteve, your last tip (precompiling) did the trick for me, and I think it's the cleanest way to do so! Thanks! |
@pietere, the only problem is that, since it's creating precompiled assets without the digest, your development environment will actually serve those instead of the live ones in your app directory, which can really mess things up when developing. So, in order to use But I agree, it's the cleanest way to do so (just not the most convenient). |
@JangoSteve, yes I realised that after posting my message here. I will try to find a workaround for this to automate this clean up, and post it here if I did find anything good to share |
@JangoSteve I think I have a quite clean solution
config.assets.digest = false
config.assets.prefix = "/cucumber_test_assets"
Now, each time that we run cucumber, the assets are precompiled in a folder called public/cucumber_test_assets, and development environment is not affected, so no need to clean up those assets.
public/cucumber_test_assets/ |
This works fine with simple asset directory structure ( application.css ), but I am working on a multi-layout app, and I my application.css is not used ... rather I have 2 css assets files referenced in my layouts : each one requiring different css files .... it works fine in dev / prod .. |
found it : adding other files .... config.assets.precompile += ['admin.js', 'admin.css', 'swfObject.js'] |
Combining @pietere solution with https://github.com/dnagir/guard-rails-assets works like a charm :) |
Why not release a gem named capybara-rails that adds the rails-specific logic, just like many other gems out there? cc @JangoSteve |
@maxim, the biggest factor is that the original solution requires re-writing parts of But if one of the newer solutions can solve the problem without rewriting the internal methods, then I'd say yes, creating a separate gem would be a good approach. |
@JangoSteve I'm sure @jnicklas wouldn't mind adding an extension point where one is useful. On the other hand, I don't see a problem overriding a method in capybara as long as dependencies are correctly specified. We can just add Regarding the feature being Rails-specific — this pull request would be, but a generic extension point would not. Rails is nothing more than a very popular type of Rack app that has some special handling of assets. I see it as a good enough reason to add an extension point to inject "special handling of assets". There are rack apps in the wild that depend on it. Whether or not |
@maxim Agreed on all points. By the way, IIRC, when I originally wrote this, I actually was going to just extend But maybe that's not a bad tradeoff to gain the extensibility and modularity, especially considering this is a debug function that's not going to be used inside loops or in production apps. |
FWIW, 👍 for |
try the capybara-screenshot gem. Works well. |
Was this ever further developed? I don't see a (working) Also, unfortunately @JangoSteve 's solution doesn't seem to work for me. I can see the assets being pre-compiled in in the <link href="/assets/application.css" media="all" rel="stylesheet" type="text/css"> But the css simply isn't loaded. Perhaps, I need the full path in there for it to work, because the base-path is probably incorrect when opening the html page with Capybara? EDIT My huch was correct, the following does work: <link href="../../public/assets/application.css" media="all" rel="stylesheet" type="text/css"> But I only tested this using the Chrome inspector and editing the path manually. Now I need to find a way to make this change permanent. Although I don't understand why others don't have this issue. |
@JeanMertz I don't believe it has been developed. I might put in some work on this tonight, however. |
@sdhull Thank you for your input. As long as the above asset-precompile option works (hint: for me it doesn't, see my above updated post), I guess the need is a bit less relevant. Still, a gem would make it easier for most. |
@JeanMertz |
@sdhull precompiling works, but the path to the precompiled assets is "wrong". It would not be wrong, if the page was to be opened using a web-server, which root path would be set to
And then the assets are linked like this: <link href="/assets/application.css" media="all" rel="stylesheet" type="text/css"> Obviously, that isn't going to work, because there is nothing setting the root path to So again, not sure why others have gotten this to work, but I don't see how this would work without a change to the root path? (I am not using the fork in this PR, but it was my understanding that using the asset precompiling solution doesn't require a fork, correct?) |
Ahhh, yes ok that makes sense. Not sure what the recommended solution is at this point. I'm running capybara 1.1.4 which rewrites asset paths to file paths like (in OSX): <link href="/Users/sdhull/path/to/app/public/assets/application.css" media="all" rel="stylesheet" type="text/css"> You must be running a more recent version of Capybara, in which this capability has been removed. I guess perhaps the recommended solution is to switch to Selenium driver and throw a debugger in your example to pause execution so you can inspect & debug your page in the browser window that pops up during execution. Also, FWIW, I prefer debugging in Chrome, so we have this in our Capybara.register_driver :selenium do |app|
Capybara::Selenium::Driver.new(app, :browser => :chrome)
end unless ENV["CHROME"] == "false" |
Ah, right, that makes sense. I was actually debugging by looking at the Capybara method documentation but only now do I realize that documentation is for version One question: I was considering doing something like that, but I feel like I might lose a lot of performance using any other driver than |
If you choose to embark on that journey of adding back asset rewriting, you should consider going whole-hog and adding asset server support. Maybe via a Of course, I'm working on adding it as a new PR, following the guidelines outlined by @jnicklas in #609 (after my brief but furious outrage about the asset situation in And if my PR doesn't get merged, then I figured I'd release it as a gem that monkey-patches it in. |
That seems like a great solution. I'll be sure to chime in if you have anything to go with (just ping me at Github). For now, I'll use a dirty method overwrite to at least get me on the road again. Finally, about the slower test suite when swapping |
Nope, we use Also worth noting is that for non-js specs, assets really don't matter that much for us anyway. If a spec fails, it's generally because something wasn't output on the page (or was output when it wasn't supposed to be), in which case assets don't matter much. Only final annoyance for us is that we like to grab screenshots using We were previously trying to use |
Thank you for your input. Did you check out jonleighton/poltergeist? Maybe that would solve your problems, it seems like a nice alternative to |
Holy crap, |
For future reference, here's the method I added back from earlier Capybara versions: module Capybara
class Session
def save_page(path=nil)
body = rewrite_css_and_image_references(html)
path ||= "capybara-#{Time.new.strftime("%Y%m%d%H%M%S")}#{rand(10**10)}.html"
path = File.expand_path(path, Capybara.save_and_open_page_path) if Capybara.save_and_open_page_path
FileUtils.mkdir_p(File.dirname(path))
File.open(path,'w') { |f| f.write(body) }
path
end
private
def rewrite_css_and_image_references(response_html) # :nodoc:
return response_html unless Capybara.asset_root
directories = Dir.new(Capybara.asset_root).entries.inject([]) do |list, name|
list << name if File.directory?(name) and not name.to_s =~ /^\./
list
end
response_html.gsub(/("|')\/(#{directories.join('|')})/, '\1' + Capybara.asset_root.to_s + '/\2')
end
end
end Simply add it in some file like This overwrites the default def save_page(path=nil)
+ body = rewrite_css_and_image_references(html)
path ||= "capybara-#{Time.new.strftime("%Y%m%d%H%M%S")}#{rand(10**10)}.html"
path = File.expand_path(path, Capybara.save_and_open_page_path) if Capybara.save_and_open_page_path
FileUtils.mkdir_p(File.dirname(path))
File.open(path,'w') { |f| f.write(body) }
path
end |
@JeanMertz Woot! Check me out: #958 |
@JeanMertz You cannot use super, cuz it's not inheritance. But there's another option - alias_method_chain. Something like: def save_page_with_assets_patch(...)
end
alias_method_chain :save_page, :assets_patch Or without active_support: alias_method :foo_without_feature, :foo
alias_method :foo, :foo_with_feature |
I added support for the Rails 3.1 asset pipeline to
save_and_open_page
.It's pretty straight forward. Currently, the method rewrites local asset paths to absolute filesystem paths in the rendered HTML. So, I basically just added a check to see if
::Rails
is defined, if the version is 3.1 or greater, and if the asset pipeline is enabled. If so, it loads the asset manifest file to grab the asset fingerprints for each file, and then rewrites the fingerprints into the rendered HTML.I have not yet extensively tested this, or had a chance to think about what a test case would look like, but it works very well with my app.