-
Notifications
You must be signed in to change notification settings - Fork 3.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
WIP: test: jasmine-ify original tests #1129
Conversation
describe('integration suite', function () { | ||
it('should put plaintext into a paragraph', function () { | ||
expect(marked('Hello World!')).toBe('<p>Hello World!</p>\n'); | ||
}); |
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.
We don't need to keep this test if we don't want to. I just added it as an example
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.
Eventually it'll go under a describe('plaintext') or something.
These are based on test/original/links_inline_style.md
afab39f
to
a693f23
Compare
So these tests will be prettier if we can write code like this: expect(marked(`[${desc.text}](${desc.link})`)).toBe(`<p><a href="${des which is not "old node"-compliant (hence the travis failure). Thoughts? |
Ya we need to stick to es5 in the tests until we figure out a build step or drop node <6 |
I think the idea is v2.0 we will support only evergreen browsers and node >=6. At that time we will switch everything to ES6+ |
@davisjam: Can't really read that. Newb-light is target. |
@joshbruce Acknowledged. |
These are based on test/original/inline_html_simple.md
20d0ef7
to
b681158
Compare
@joshbruce Is the code in b681158 more readable? |
@davisjam: Try one test per example?? See also: #1124 (comment) |
@joshbruce Hesitant to do that since there would be a ton of boilerplate. Thoughts? |
@davisjam: I might need to slow down a bit in response time...really jazzed to see all the cooperation and collaboration...y'all are awesome! ...question: Define "boilerplate" in this context? Considerations (imho):
Having said that, I'm not a big fan of writing tests for their own sake...the fact we're doing this against a spec makes it a little more awkward than what I was doing there at the moment...partially, I think because our renderer is slightly complex compared to the type definition of an HTML element per #1102 (comment) - which is what I ended up with after starting with a PHP associative array (JSON). It makes the NPM package size bigger, but that's not what people are using operationally in their sites...if I have a question on why something is happening; instead of running to GitHub, I can run to the tests or spec (make sure I'm suing the right combination, make sure it's supported, then maybe post a PR to make the test pass). Something like that...?? |
Yes. I'm writing tests at the level of "marked honors inline html" or "marked honors inline links". Spec-by-spec. |
9d2779a
to
e86f097
Compare
These are based on test/original/auto_links.md
Okie dokie, I'll leave this for now. @joshbruce @UziTech Let me know how the current version looks. |
These are based on test/original/amps_and_angles_encoding.md
Currently 'npm run lint' does not scan the new tests.
Can we just replace "test/index.js" with "test/"? Seems to work. |
@davisjam really we should add an |
]; | ||
|
||
samples.forEach(function(sample) { | ||
expect(marked(sample.md)).toBe(sample.html); |
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.
We should figure out how to get a better error message other than expected 'some really long string' to equal 'some other really long string'
.
We could try something like jasmine-diff-matchers
@@ -38,7 +38,7 @@ | |||
"uglify-js": "^3.3.10" | |||
}, | |||
"scripts": { | |||
"test": "jasmine --config=jasmine.json", | |||
"test": "node test && jasmine --config=jasmine.json", |
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.
test/specs/specs-spec.js
already takes care of running the test/index.js
tests in jasmine so there is no need to run node test
.
{'md': 'Inline [link](/script?foo=1&bar=2) with ampersands', 'html': '<p>Inline <a href="/script?foo=1&bar=2">link</a> with ampersands</p>\n'}, | ||
{'md': '[link with ampersands](http://example.com/?foo=1&bar=)', 'html': '<p><a href="http://example.com/?foo=1&bar=">link with ampersands</a></p>\n'}, | ||
{'md': '[link title with ampersands](http://att.com "AT&T")', 'html': '<p><a href="http://att.com" title="AT&T">link title with ampersands</a></p>\n'}, | ||
]; |
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.
I think it would be easier to just pull these from their current files. I hate hard coding strings in javascript. Too much possibility for errors.
We could construct each test from the files and add the describe
and it
descriptions to the front-matter in the .md file
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.
or maybe reorganize these tests and pull the describe
description from the folder name and the it
description from the file name
See #1159 |
I think we should try to converge on a similar test pattern as #1160, adding a json file for the original test similar to commonmark.json |
I think if we're going to Jasminify the legacy tests, we might be covered elsewhere. #1160, for example, should/would cover the CM tests; so, they can be removed. The GFM ones would be covered by the GFM tests that haven't been created yet. There may also be conflicts between the specs and what marked does ( Point being I suppose that should we keep these around just because they've always been around? or, can we rationally deprecate (some, most, many of) them in favor of something more robust? |
I like the idea of just having CommonMark and GFM tests and deprecating all other features in favor of making marked more extensible and less bloated. We should probably keep the tests around until the options are removed (in 1.x or 2.x) |
I'm closing this in favor of #1444 |
Jasmine-ifying original tests.
So far:
amsps_and_angles_encoding
auto_links
links_inline_style
inline_html_simple