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

Update CommonMark test to 0.30 #2217

Closed
vinicius73 opened this issue Mar 3, 2022 · 4 comments · Fixed by #3292
Closed

Update CommonMark test to 0.30 #2217

vinicius73 opened this issue Mar 3, 2022 · 4 comments · Fixed by #3292

Comments

@vinicius73
Copy link
Member

vinicius73 commented Mar 3, 2022

Now it is used 0.28 (2017-08-01) version of CommonMark Spec in tests.
But there are two new releases
Actually the last version is 0.30, and this is supported by markdown-it

I've tried a fast update, but some tests failed.

Also, do we really need to test it? markdown-it already test it.

@vinicius73 vinicius73 added the enhancement New feature or request label Mar 3, 2022
@juliusknorr
Copy link
Member

Actually 0.29 should currently be in use d7de7d9

The reason those tests were added is mainly because text does multiple conversion steps

  • markdown to html
  • html to json to fit into the prose mirror schema
  • prose mirror to markdown

The tests were mainly to ensure that with those different intermediate steps, we still stick to the expected format. https://github.com/nextcloud/text/blob/master/src/tests/markdown.spec.js#L25-L49

There were some test failures last time only due to additional whitespace which were skipped in the linked spec file, so might be worth to have a look at the actual failures if those are also related to that or something more serious.

@vinicius73
Copy link
Member Author

Thanks for the explanation @juliushaertl
I will take a close look at it when I can.

@susnux
Copy link
Contributor

susnux commented Jun 21, 2022

The tests were mainly to ensure that with those different intermediate steps, we still stick to the expected format. https://github.com/nextcloud/text/blob/master/src/tests/markdown.spec.js#L25-L49

But the commonmark tests only test markdown-it, they do no test the markdown->html->markdown pipeline, that would require using markdownThroughEditor. So they are not that useful as markdown-it tests for commonmark itself.

@julien-nc
Copy link
Member

Any news on that? @vinicius73 Do you still feel it's worth updating? If not, let's close this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants