aanand / deadweight

NOT MAINTAINED
MIT License
1.19k stars 52 forks source link

Rails 4 support #22

Open jmuheim opened 10 years ago

jmuheim commented 10 years ago

I never used this gem before, but I guess that it's not working anymore for newer versions of Rack and/or Rails, see especially my changes in lib/deadweight/rack/capturing_middleware.rb (respond_to :body doesn't seem to be correct anymore) and in lib/deadweight/hijack/rails.rb (application.css is no longer under stylesheets, but in assets, and it needs to be precompiled).

I didn't find a more elegant way to load application.css into DeadWeight than to do a manual rake assets:precompile from within Ruby. It would be nice to use the same source that Rails generates when a js: true test is run, but this is done only the first time such a request test is run, so we don't have it at this early stage. Maybe there is a way to trigger the generation manually somehow?

I also refactored the console output: I don't think it's useful to hijack the log and send it to a file. I want to see what's happening with my tests, e.g. how many % already are done, etc.

screenshot_05_01_14_22_34

It would be nice to prevent the output of rake assets:precompile from being displayed, but I didn't find a way to do it.

As I only use this as a Rails gem, I didn't verify that all other functionality's still working.

aanand commented 10 years ago

First off, thanks for contributing!

:+1: to the Rails-related changes - no-one's actually told me they're using the hijack functionality, so I don't think there's any need to worry about backward compatibility, and it'd be good to have it working with Rails 4.

I also agree that it's probably better not to hijack stdin/stdout - it's a big hack, and other tools that hook into test suites (e.g. simplecov) don't do it, so it's probably a bit surprising.

Something seems to have gone wrong with the Travis build - was that you?

I have one issue with the changes you've made to the output. I'm fine with waiting until processing has finished to output anything (especially when there's a test suite running), but comma-separated selectors are less trivial to manipulate with command-line Unix tools than line-separated. (Relatedly, if the test suite were running, I wouldn't expect it to pass, as it splits the output by line.)

In addition, it's useful to be able to see what selectors are actually used in each CSS file for debugging purposes, though that needn't necessarily be on by default.

Still, it's nice that your version takes up a lot less vertical space. Perhaps when running in hijack mode, we should log Deadweight's normal stdout (unused selectors) and stderr (stylesheets + used selectors therein) to two files, and just print "Found X selectors, Y used, Z unused" to the terminal.

What do you think?

jmuheim commented 10 years ago

Something seems to have gone wrong with the Travis build - was that you?

Yeah I guess that's me. I didn't run the specs before pull-requesting as I wasn't sure whether the gem's still maintained and didn't want to put a lot of effort into it. But I will take a look at the specs soon, knowing now that you still care about deadweigth. :+1:

it's useful to be able to see what selectors are actually used in each CSS file for debugging purposes, though that needn't necessarily be on by default.

A typical Rails app only has on application.css file, so I optimized it for it. But I'm sure we could group the output by scanned CSS files somehow.

just print "Found X selectors, Y used, Z unused" to the terminal

Sounds interesting. I will take a closer look at it.

jmuheim commented 10 years ago

By the way, I'm working on a gem called Headhunter that automatically validates the HTML responses of Rack apps (using Tidy HTML). It was heavily inspired by Deadweight.

I'm thinking about consolidating Deadweight's functionality into Headhunter to make it become a full-blown helper when trying to develop valid and clean HTML and CSS.

jmuheim commented 10 years ago

Hm it seems that the specs run through without a problem:

josh@macbuech:~/Documents/Work/MuheimWebdesign/deadweight (master *)$ rake
/Users/josh/.rvm/rubies/ruby-2.0.0-p353/bin/ruby -I"lib:lib:test" -I"/Users/josh/.rvm/gems/ruby-2.0.0-p353@transition/gems/rake-0.9.2/lib" "/Users/josh/.rvm/gems/ruby-2.0.0-p353@transition/gems/rake-0.9.2/lib/rake/rake_test_loader.rb" "test/cli_test.rb" "test/deadweight_test.rb" "test/rake_task_test.rb" 
Run options: 

# Running tests:

Finished tests in 1.997180s, 7.5106 tests/s, 17.0240 assertions/s.                                                              
15 tests, 34 assertions, 0 failures, 0 errors, 0 skips

ruby -v: ruby 2.0.0p353 (2013-11-22 revision 43784) [x86_64-darwin13.0.0]

Is there anything else that could have disturbed CI?

aanand commented 10 years ago

Travis seems happy now, so that's good.

jmuheim commented 10 years ago

I have changed the output to newlines again and override it for Rails. :+1:

In addition, it's useful to be able to see what selectors are actually used in each CSS file for debugging purposes, though that needn't necessarily be on by default.

For this I don't have time at the moment, because (as mentioned before) I'm writing my own HTML&CSS-Validator at the moment.

jmuheim commented 10 years ago

Just wanted to mention that the first version of my Headhunter gem is out! For Rails projects, I think it's a very powerful supplement (or even replacement) for your gem.

From the docs:

Headhunter is an HTML and CSS validation tool that injects itself into your Rails feature tests and automagically checks all your generated HTML and CSS for validity.

In addition, it also looks out for unused (and therefore superfluous) CSS selectors.

This is all done locally, so no external service is used.

Would be great to get some feedback from you. I'm currently working on adding some functionalities from your open pull requests into it which I also stumbled across meanwhile (as I copy&pasted a lot of your code :wink: : ).

I hope you're not mad that I'm trying to make your Deadweight gem obsolete...

Headhunter

aanand commented 10 years ago

PLEASE MAKE DEADWEIGHT OBSOLETE.

I'll take a look when I've got the time. Sounds great. Can it be decoupled from Rails/Ruby?

jmuheim commented 10 years ago

Can it be decoupled from Rails/Ruby?

At the moment it's "married" to Rails. But it wouldn't be much of a problem to "divorce" it. As soon as the Rails version works well, I will think about adding a command line version (like you did with Deadweight).