-
Notifications
You must be signed in to change notification settings - Fork 18
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
Add tests #16
Add tests #16
Conversation
AppVeyor is SO SLOW |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good.. leaving just a couple of comments
actual = commonmark.convert("https://example.com") | ||
expected = "https://example.com" | ||
expect(actual).to match(expected) | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO, there should also be a test for Markdown links
commonmark.convert("[example](https://example.com)")
commonmark.convert("[example][link]\n\n[link]: https://example.com")
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remember that we're not testing the MarkDown processor itself, merely our interface to the MarkDown processor. If the MarkDown processor does not conform to the CommonMark spec, its own tests will reveal that. 👍
That said, if there is a reason to add this test, it would not be difficult.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah I see.. is there any aspect where our interface markedly differs from kramdown
? If yes, then that should definitely be tested (will serve as documentation) because the HTML markup is going to change when users switch to this parser..
spec/spec_helper.rb
Outdated
TMP_DIR = File.expand_path("../tmp", TEST_DIR) | ||
|
||
require "jekyll" | ||
require File.expand_path("../lib/jekyll-commonmark.rb", TEST_DIR) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can't we use require_relative "jekyll-commonmark"
instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#TIL 😻
yeah.., also the fact that it doesn't allow running parallel builds for the free accounts further increases the total time.. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! 😃
@jekyllbot: merge +dev |
PR automatically created for @pathawks.
Fixes #14