CommonMark Tests

[joshbruce]

Marked version: 0.3.18

Markdown flavor: CommonMark

Description

• Fixes Build CommonMark test bed #1159

Putting a script together in PHP (it's what I know and this should be a one time deal) to generate the test cases. However, the test harness does not seem to run all files in the integration folder. Given the number of test cases from the specs, think it would be better to divide the tests by flavor and section.

@UziTech: Thoughts??

Contributor

• Test(s) exist to ensure functionality and minimize regresstion (if no tests added, list tests covering this PR); or,
• no tests required for this PR.
• If submitting new feature, it has been documented in the appropriate places.

Committer

In most cases, this should be a different person than the contributor.

• Draft GitHub release notes have been updated.
• CI is green (no forced merge required).
• Merge PR

[UziTech]

1. Where did you get the JSON file? Is that from CommonMark and what version of the spec?

2. I like the idea of constructing the tests from the JSON but we should not add another manual build/test/publish step. And we definitely don't want to require any maintainers to install php in order to update the tests.

[joshbruce]

@UziTech:

1. @Feder1co5oave gave it to us back on Attempting CM 331-338 #1124 - it's CommonMark 0.28 (not sure if an equivalent exists for the GFM extensions). http://spec.commonmark.org/0.28/spec.json

2. (Not sure where the extra step is coming up.) I'm only using the PHP to generate the tests that I then copy and paste into the JS file. Once all the tests are accounted for - no more PHP - I didn't have to do it the "long" way (what I and @davisjam were doing before). If the JSON converter was going to stick around I might figure out how to do it in JS...since I only intend to keep it around long enough to create the test bed I did it the easiest way I knew how - PHP's standard library (and PHP was built to spit out strings).

3. My question or request at the moment is to make the integration test run all files inside the folder (well all the tests running all the files within a given directory). Because there will most like be half a dozen sections: cm_028_tabs.js, cm_028_thematic_breaks.js, cm_028_headings.js, and so on. And, each section will have half a dozen tests or so.

[UziTech]

The manual build step comes from creating the tests every time a new version of the spec comes out.

I say we should just create a script to load and run the json file instead of building the tests. That way when a new version of the spec comes out all we have to do is copy the new json file.

[joshbruce]

@UziTech: Part of me wants to agree with that. For examples we aren't going to comply with are you thinking we would have something like an array of examples we aren't complying with?

var noComply = [];
var shouldComplyButFail = [];

[UziTech]

We could add something to the json like:

{
  markdown: ...,
  html: ...,
  noComply: true,
  ...
}

[joshbruce]

We could. Though, if we have to modify the JSON I think we would be slipping back to a pre-build step, yeah? 0.29 gets released, we modify that JSON. I could take what @davisjam has put together.

1. Run the tests...by itself won't result in merge because Travis will fail.
2. Create a shouldPass array to denote broken examples...ignore them so Travis will pass.
3. Create a willNotComply to denote rules that are too complex for us to resolve (some of the em rules identified by @Feder1co5oave, for example).

Doesn't require modifying the JSON. Allows test bed to cover all the things while indicating broken things and examples we have no intention of covering. This would put a little bit more burden back on the developers to read the JSON to find out exactly what we're not in compliance with (something they get for free by doing it the longer way).

@UziTech: Check the spec now. 407 failures, if I didn't mess something up, which is quite possible. Basically Marked only passes 30% of the spec.

[joshbruce]

@UziTech: Been looking at some of the failing tests. Looks like it could be an XHTML versus HTML5 issue. For self-closing elements XHTML says to add a forward slash, HTML5 doesn't have this requirement. It looks like Marked is spitting out the proper HTML5 but the HTML from the spec has the trailing slash.

407) CommonMark 0.28 should pass example 551
  Message:
    Expected '<p><img src="url" alt="foo"></p>
    ' to be '<p><img src="url" alt="foo" /></p>
    '.

Debating on have the script remove the trailing slash. Thoughts??

[UziTech]

hmm ... I'm thinking this might be a better question for @Feder1co5oave.

Would it be easier to remove the slashes from the json file every time we update the spec? or would it be easier to add slashes in marked to copy the spec?

I don't think the spec requires them.

[UziTech]

A third option could be coming up with some AST sort of approach for comparing them instead of comparing strings.

[joshbruce]

1. Removing the slash from the JSON should be a matter of string replacement...can't imagine it being too difficult.
2. Might be able to set the XTHML option to true?? Right now it's marked out of the box, which is X?HTML false. Having said that, not sure if our XHTML implementation uses the same spacing.
3. Lost me on the acronym...AST??

[Feder1co5oave]

Enabling XHTML mode solves the closing slash issue. I used this in my last pr. Space is ignored by the current comparing algorithm. My old pr to use html-differ uses parse5 under the hood to compare html, that's an AST. Il mer 28 mar 2018, 01:35 Josh Bruce <notifications@github.com> ha scritto:

…

1. Removing the slash from the JSON should be a matter of string replacement...can't imagine it being too difficult. 2. Might be able to set the XTHML option to true?? Right now it's marked out of the box, which is X?HTML false. Having said that, not sure if our XHTML implementation uses the same spacing. 3. Lost me on the acronym...AST?? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1160 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AByGhNz91c2Jz2nC6iQRSwPTxuBabsCNks5tisy1gaJpZM4S5wuY> .

[joshbruce]

1. Latest attempt (f144566) shows 624 with 379 failures.
2. Changing the configuration option to use XHTML gave mixed results. Same number of failing tests before but not the same tests.
3. Interesting. What does AST stand for in this context (abstract syntax tree??)? It appears the Jasmine tests include whitespace comparison. Some of the failing tests are because of new line characters.

[joshbruce]

Added html-differ and jasmine-custom-message. Brings us to 284 failures. Next step will be the ignores.

Would like to get feedback, if any, on this as a viable solution for the future from @UziTech and @Feder1co5oave. Tagging in the rest of the core team as well.