at-import / Sassy-Maps

Map helper functions for Sass 3.3 and up
MIT License
66 stars 22 forks source link

Remove Compass dep, Grunt tasks, Setup SASS_PATH. #6

Closed metaskills closed 10 years ago

metaskills commented 10 years ago

Thanks again Sam for allowing me to contribute :heart:

metaskills commented 10 years ago

OK, that got the test suite green again. I also added Ruby 2.1.1 to the list too.

metaskills commented 10 years ago

@Snugug It would be epic if this could get merged soon. I'm gonna starting bundling our gem to a few applications today and it would be nice if I did not have to point each Gemfile to our branch. Any feedback?

FYI, the built in Bundler tasks for publishing a gem are required in the Rakefile. So assuming a post merge, then bumping the version files to 0.3.3, you would see this:

$ rake -T
rake build    # Build sassy-maps-0.3.3.gem into the pkg directory
rake install  # Build and install sassy-maps-0.3.3.gem into system gems
rake release  # Create tag v0.3.2 and build and push sassy-maps-0.3.3.gem to Rubygems
rake test     # Run tests
Snugug commented 10 years ago

It's a lot of changes, the earliest I can probably go through it is Tuesday

On May 30, 2014, at 8:14 AM, Ken Collins notifications@github.com wrote:

@Snugug It would be epic if this could get merged soon. I'm gonna starting bundling our gem to a few applications today and it would be nice if I did not have to point each Gemfile to our branch. Any feedback?

FYI, the built in Bundler tasks for publishing a gem are required in the Rakefile. So assuming a post merge, then bumping the version files to 0.3.3, you would see this:

$ rake -T rake build # Build sassy-maps-0.3.3.gem into the pkg directory rake install # Build and install sassy-maps-0.3.3.gem into system gems rake release # Create tag v0.3.2 and build and push sassy-maps-0.3.3.gem to Rubygems rake test # Run tests — Reply to this email directly or view it on GitHub.

Snugug commented 10 years ago

Just ran a quick test of the new Rake tests, here are some thoughts based on what is available in Navigator:

metaskills commented 10 years ago

Diffs need to be generated (not in the command line, physical diff) on a failure

Can you go into detail on that more? A "diff" is made and presented when the test fails. Oh... are you asking that diff to be written to disk? I can do that, got a place in mind? Maybe tests/diffs and add that to the .gitignore too?

Tests need to be automatically discovered.

OK, doing that now.

Tests need to be able to be nested n folders deep inside the tests directory in order to be organized

OK.

Provide a way to generate control files from Sass tests for initial testing and checking. Many times for CSS it's easier (and less error prone) to generate a CSS file for a test and do an initial manual check instead of writing the test control by hand.

Let me think on that for a second and get back to you.

Snugug commented 10 years ago

In Navigator, if there is an error, in the output folder (where the test files get generated to in order to compare), if there is a failure, a {test}.css.diff that gets generated next to {test}.css to see the diff.

Having just run a Navigator test, a little visual update would be nice to the current tests. Take a look at what test results look like for Navigator:

Navigator test output

metaskills commented 10 years ago

screen shot 2014-05-30 at 4 44 54 pm

Snugug commented 10 years ago

Looking good. A couple of things:

Once these fixes are checked and there's a rake command for compiling controls, this should be good to merge in. Thank you much for all of this, it's awesome!

metaskills commented 10 years ago

Likewise, if everything goes well, you see: screen shot 2014-05-30 at 4 47 18 pm

Provide a way to generate control files from Sass tests for initial testing and checking.

I can make a small rake task that takes a file name arg of a recent addition to tests/tests and then compiles it tests/control. Likewise, I could get very clever and use git ls-files when the suite starts. Any test file that does not have a versioned control would just generate a new control. The later seem kind of pointless, it would never fail. Thoughts?

metaskills commented 10 years ago

a rake command for compiling controls

OK.

Put [x/n] test number next to each test (in color)

I do not know how to do that.

If there is an error, inform the user where they can find the .diff file

OK.

Snugug commented 10 years ago

No need to get super clever.

I'd prefer it to simply recompile everything; it provides a super quick sanity check that everything is working as expected (you'll see git changes, much faster than running the test suite) and is useful when doing a full refactor (which I've done a couple of times)

metaskills commented 10 years ago

OK, that was exhausting, but I think everything is done. Am I on Team-Sass now? :)

screen shot 2014-05-30 at 6 07 31 pm

metaskills commented 10 years ago

Output from rake render: screen shot 2014-05-30 at 6 12 27 pm

Snugug commented 10 years ago

A couple things before we're good to go with this:

metaskills commented 10 years ago

The order tests are run changes for every test. In theory this isn't an issue, but it's confusing and should be consistent.

Minitest runs tests in random order for a good reason. It can be configured to not do that, but it is a self deprecating name. How about just removing the numbers from the files and let MiniTest just run better self descriptive tests?

Tests names should have [x/y] in color before each.

I said earlier that I do not know how to do that.

metaskills commented 10 years ago

For instance, test/foo_bar and tests/foo/bar would both be called tests_foo_bar but should be called foo_bar and foo/bar but, worse yet, are treated as the same item in the test runner.

I totally get it, but that just seems like a downside of generating test names from files automatically. Remember, method signatures can not have "/" in them – that's just Ruby.

Currently I have a fair amount of time on the current implementation. I really like a lot of the small changes despite some being orthogonal to what may be technically needed. That said, I love where we are and have enjoyed the work, this stuff is jazzy man. Besides ordering the tests (which I just pushed up), everything else is getting to writing a custom Test::Unit/Minitest runner, which is not beyond my capacity, but would take me a few hours to do. Is this needed?

metaskills commented 10 years ago

@Snugug I should point out I am more than willing to help you continue building our Navigator replacement and even abstracting it to a proper gem with good deps so it can be used on other Team-Sass projects. Can that happen out of the context of this pull request?

Snugug commented 10 years ago

I must have missed that you didn't know how to do the numbering. Running the tests in order isn't so much about the tests needing to be run in order as it is off-putting to see numbered tests run out of order. The numbered are there to keep my head sane and be able to tell at-a-glance if a test is still failing because it runs in the same place. CSS regression testing (what we're doing) is all self-contained anyway, so there's no reason they need to run randomly, each test will always be self-contained.

The only thing that needs to be resolved is the file naming issue. The way were were able to do it in navigator is by simply asserting for each, we don't need to define a unique test for each. That may not be the right way, but it seems to work. If we cannot have slashes in the name, there needs to be some other non-ambiguous way of combining on folders that we can use as that will be an issue.

I'm sorry that this process is frustrating. I'm going to absolutely ask you to help bring this into Navigator, but as this is being proposed as a replacement for Navigator now in this PR, it needs to have (as close to) feature parity with Navigator as possible, and having ambiguous tests is a big issue where it doesn't. The other issues are, agreed, superficial and don't need to be complete for them to be pulled in, but this one does.

metaskills commented 10 years ago

I'm sorry that this process is frustrating. I'm going to absolutely ask

Cool, I am glad to help.

The only thing that needs to be resolved is the file naming issue. The way were were able to do it in navigator is by simply asserting for each

I did look at the Navigator code when I implemented the new code. Check out this link:

https://github.com/Team-Sass/navigator/blob/master/tests/.unit_tests.rb#L18-L21

That code does not account for duplicate file naming conventions in different folders. I do think it is clever that it uses a begin/ensure block to put additional messaging where I just opted to put all the information in the flunked message.

The only thing I see that is different is the usage of some special "reporter" that gives you the taps style numbering. Let me look into that a bit, but right now I am not seeing it. Perhaps there is an alternate reporter that I do not know about. Will report back in a sec...

metaskills commented 10 years ago

OK, it took me a few hours, but I learned about Minitap and TAPOUT for custom reporters.

Tapout's RuntimeReporter seemed like it had most of what you wanted but it had this really ugly backtrace which always showed the stack trace source path leading back to Navigator's define_method which was 30 or 40 lines of useless info during a test failure.

So I learned how to make a custom subclass of that reporter called NavigatorReporter and then how to use it from the rake task. Sooooo here is where we are at:

screen shot 2014-05-31 at 9 03 08 pm

metaskills commented 10 years ago

I just removed 1.8.7 from TravisCI since the reporters are not compatible. That should be fine, Sass is tested to 1.8.7 and we are not testing Sass, just our code on top of Sass.

metaskills commented 10 years ago

Happy Monday Morning Sam! Is today the day?

Snugug commented 10 years ago

ONE last thing: Please clean out the output folder on each run, otherwise artifacts from previous runs may be in there.

Otherwise, looks fantastic! Thank you so much for your work!

metaskills commented 10 years ago

Done deal. I played around by adding this to the reporter, but that did not work since the reporter just reads the TAP output stream and knows nothing about Navigator. I also tried doing it in a setup hook, but does not work either since it will run the clean output dir on each tests case. I tested this method and it works no matter what. A test run cleans the output dir before anything else and only once.

metaskills commented 10 years ago

Happy Tuesday Sam! Is today the day? If so, the timing would be great for a new gem release. Also, when you pull this in, let me know where and how we should do some work on making a true abstract Navigator replacement. I would be happy to put some of my free time to that so it can just be leverages as a gem in this and other projects.

Snugug commented 10 years ago

Done. Do you need a new gem rolled or can you work off of the GH repo?

metaskills commented 10 years ago

Thanks! New gem please!

Snugug commented 10 years ago

0.4.0 pushed! Thank you so much for your help!