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

Bump kramdown, rake, shoulda, cucumber, redcarpet #744

Merged
merged 3 commits into from Jan 30, 2013

Conversation

Projects
None yet
6 participants
@pusewicz
Copy link
Contributor

pusewicz commented Jan 9, 2013

Improving for the future FTW!

@mattr-

This comment has been minimized.

Copy link
Member

mattr- commented Jan 9, 2013

There's several major version bumps here. Can we see the output of
your test run to ensure that nothing broke? Thanks! ❤️

@pusewicz

This comment has been minimized.

Copy link
Contributor Author

pusewicz commented Jan 10, 2013

Actually I just noticed that the test-unit specs are being skipped.

Screen Shot 2013-01-10 at 07 24 43

@pusewicz

This comment has been minimized.

Copy link
Contributor Author

pusewicz commented Jan 10, 2013

@travisjeffery Do you know anything about changes to shoulda that could break this?

@travisjeffery

This comment has been minimized.

Copy link

travisjeffery commented Jan 10, 2013

@pusewicz that seems pretty weird that it just skips them, I can't think of what it could be right now. I'm doing nothing but sitting in an airport later today so I can mess around and check it out more in-depth then.

@pusewicz

This comment has been minimized.

Copy link
Contributor Author

pusewicz commented Jan 10, 2013

That would be great!

Sent from my iPhone

On 10 Jan 2013, at 18:44, Travis Jeffery notifications@github.com wrote:

@pusewicz that seems pretty weird that it just skips them, I can't think of what it could be right now. I'm doing nothing but sitting in an airport later today so I can mess around and check it out more in-depth then.


Reply to this email directly or view it on GitHub.

@mattr-

This comment has been minimized.

Copy link
Member

mattr- commented Jan 10, 2013

This is not a problem with shoulda from my point of view. I downgraded
shoulda back to the previous version that Jekyll was using and kept
everything else the same and the unit tests were still skipped.

@travisjeffery

This comment has been minimized.

Copy link

travisjeffery commented Jan 10, 2013

@mattr- yeah my intuition said the same thing. I haven't gotten any issues related to this, and them not running seems like it'd be more a jekyll config issue.

@pusewicz

This comment has been minimized.

Copy link
Contributor Author

pusewicz commented Jan 10, 2013

That is weird because I did downgrade to the previous version and it did work for me.

What about your Ruby versions?

On 10 Jan 2013, at 19:27, Travis Jeffery notifications@github.com wrote:

@mattr- yeah my intuition said the same thing. I haven't gotten any issues related to this, and them not running seems like it'd be more a jekyll config issue.


Reply to this email directly or view it on GitHub.

@pusewicz

This comment has been minimized.

Copy link
Contributor Author

pusewicz commented Jan 10, 2013

It did not work with simple rake test but it did with bundle exec rake test.

On 10 Jan 2013, at 19:29, Piotr Usewicz piotr@layer22.com wrote:

That is weird because I did downgrade to the previous version and it did work for me.

What about your Ruby versions?

On 10 Jan 2013, at 19:27, Travis Jeffery notifications@github.com wrote:

@mattr- yeah my intuition said the same thing. I haven't gotten any issues related to this, and them not running seems like it'd be more a jekyll config issue.


Reply to this email directly or view it on GitHub.

@tombell

This comment has been minimized.

Copy link
Contributor

tombell commented Jan 10, 2013

Have these updates been tested on 1.8.7, and 1.9.2?

@tombell

This comment has been minimized.

Copy link
Contributor

tombell commented Jan 10, 2013

I got the tests running on this branch by adding require 'test/unit' to the helper.rb file. However the update has broken some of the functionality and there are failing tests.

@pusewicz

This comment has been minimized.

Copy link
Contributor Author

pusewicz commented Jan 10, 2013

So we have only one test failing. I guess we can just fix the test for that.

Screen Shot 2013-01-10 at 20 48 48

@tombell

This comment has been minimized.

Copy link
Contributor

tombell commented Jan 10, 2013

Yup, looks like Kramdown just changed how it's escaping the HTML entities.

@pusewicz

This comment has been minimized.

Copy link
Contributor Author

pusewicz commented Jan 11, 2013

So shall we just fix the test?

@tombell

This comment has been minimized.

Copy link
Contributor

tombell commented Jan 11, 2013

It's a some what brittle test anyway. It shouldn't really be in this project. It's basically testing what Kramdown's own tests would be testing.

Dont test kramdown
It should have it's own tests for that
@pusewicz

This comment has been minimized.

Copy link
Contributor Author

pusewicz commented Jan 11, 2013

Cool, all green now.

@pusewicz

This comment has been minimized.

Copy link
Contributor Author

pusewicz commented Jan 14, 2013

@tombell

This comment has been minimized.

Copy link
Contributor

tombell commented Jan 14, 2013

I can't merge.

@mattr-

This comment has been minimized.

Copy link
Member

mattr- commented Jan 14, 2013

neither can I. It'll have to wait for @parkr or @mojombo

@parkr

This comment has been minimized.

Copy link
Member

parkr commented Jan 18, 2013

I'm not comfortable taking those kramdown tests out quite yet. The minor change from named to numerical HTML entities is not good enough reason to nix them. I do hope that Kramdown is testing stuff like this, but these tests are sanity tests so that we know that we're up-to-date with our configs, etc. Jekyll touches these pieces of Kramdown so I'd prefer to keep them, unless I'm totally being irrationally paranoid.

Other than that, this looks good to merge.

@pusewicz

This comment has been minimized.

Copy link
Contributor Author

pusewicz commented Jan 18, 2013

@parkr So would you prefer to put them back and update them?

@tombell

This comment has been minimized.

Copy link
Contributor

tombell commented Jan 18, 2013

@parkr yes you're being paranoid, the fact that Jekyll's tests broke when upgrading the gem shows it's testing the wrong things.

@parkr

This comment has been minimized.

Copy link
Member

parkr commented Jan 18, 2013

@pusewicz Yes, please.

@tombell I almost agree with you. If the API of a gem changes from version to version, then it'll break code, which will cause test failures. Here, Maruku's changed its behaviour and we should know about this. It's not damaging this time, but it might be good to know about in the future, if the code breaks (not just the conversion).

@tombell

This comment has been minimized.

Copy link
Contributor

tombell commented Jan 18, 2013

Testing that a gem renders markdown isn't a test for Jekyll, it's a test for the engine itself. The test is brittle and pointless being in Jekyll.

@parkr

This comment has been minimized.

Copy link
Member

parkr commented Jan 18, 2013

Ok, leave it out. I'll merge this in the morning.

@parkr

This comment has been minimized.

Copy link
Member

parkr commented Jan 18, 2013

@tombell My worry is that if our configs change and we break the renderer, we won't know.

@pusewicz

This comment has been minimized.

Copy link
Contributor Author

pusewicz commented Jan 21, 2013

@parkr Are we merging this in?

parkr added a commit that referenced this pull request Jan 30, 2013

parkr added a commit that referenced this pull request Jan 30, 2013

@parkr parkr merged commit 9b8b5b2 into jekyll:master Jan 30, 2013

1 check passed

default The Travis build passed
Details

@jekyll jekyll locked and limited conversation to collaborators Feb 27, 2017

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.