Fix tests #1002

UziTech · 2018-01-06T18:09:32Z

Changes to tests

Markdown for tests are written in .md files instead of .text
No need to --fix the tests as it is done automatically before you run the tests
Added a --no-fix arg for the tests in case you don't want it to automatically fix the tests before testing
tests that need specific options set can use front-matter at the top of the .md file to specify the options instead of the file name
fixed benchmarks
- removed robotskirt
- fixed showdown
- added markdown-it
updated readme to include new benchmarks and testing description

fixes #998

joshbruce · 2018-01-06T19:31:38Z

Wow. @UziTech, this is very impressive. Thank you so much.

Given the spread of changes I would like a second pair of eyes and some questions confirmed. Maybe @Feder1co5oave, @matt-, or @KostyaTretyak can have a look.

Questions/confirmation:

Only affects test harness and patterns (doesn't add the property setting feature requested elsewhere).
Most, if not all of the files changed or deleted are to convert them from text or HTML files to MD.
The Marked lib itself was not touched; therefore, all previous tests are in tact, benchmarks remain the same, and no new functionality was added or removed from the main lib.

Again, thank you so much, this should make it much easier to do comparitive analysis and support different specs in the future.

KostyaTretyak · 2018-01-06T19:41:37Z

test/index.js

    try {
-      fs.mkdirSync(path.resolve(__dirname, dir), 0755);
+      fs.mkdirSync(path.resolve(__dirname, dir), 0o755);


0o755 - it's not typo?

That is an octal literal

KostyaTretyak · 2018-01-06T19:48:36Z

test/index.js

+      };
+    })());
+  } catch (e) {
+    console.log('Could not bench markdown-it.');


Here we can show error like this: console.log('Could not bench markdown-it. (Error: %s)', e.message);

I was thinking of doing something like that.

Would we want to also check for e.code === 'MODULE_NOT_FOUND' and display a message like Please install x to run bench?

I think it's better to make it simpler - just error message.

@UziTech: Maybe create an issue for the idea of "module not found" - I like the idea of decoupling us from packages and giving users a message like that. I'd flag it for 0.6.0 milestone as it improves the developer experience.

The way I set it up now if they don't have a module installed they will see:

Could not bench showdown. (Error: Cannot find module 'showdown')

UziTech · 2018-01-06T20:04:35Z

@joshbruce

Yes this only changes the test harness. lib/marked.js was not touched at all
Most changed files were from removing the test/tests folder and changing all test/*/*.text files to test/*/*.md as well as setting test options with front-matter
All tests remain the same (only the way options are specified was changed)
No new tests were added as functionality did not change

joshbruce · 2018-01-06T22:36:31Z

All right. LGTM! Easy to change in future without affecting our users...rollback or something else. And confirmed...way to come together.

Again, awesome work. Thank you.

joshbruce · 2018-01-07T18:43:30Z

#746

Feder1co5oave · 2018-01-09T14:38:43Z

I recently peeked into the test script and I must say it looks hacky as hell. The fix() function especially, since its job is to make the original tests from markdown.pl work on marked. Should we consider updating these test files to work with marked out of the box? I'm actually trying to incorporate test cases from commonmark to improve out test baseline and I'm facing similar difficulties.

For instance, one of the features that marked has is automatic id generation for headings, which look quite cumbersome (and painful to hand-write) in test cases. Should we consider disabling this in testing?

KostyaTretyak · 2018-01-09T14:49:20Z

For instance, one of the features that marked has is automatic id generation for headings

@Feder1co5oave, do you mean this method?

Renderer.prototype.heading = function(text, level, raw) {
  return '<h'
    + level
    + ' id="'
    + this.options.headerPrefix
    + raw.toLowerCase().replace(/[^\w]+/g, '-')
    + '">'
    + text
    + '</h'
    + level
    + '>\n';
};

It would be better?

Renderer.prototype.heading = function(text, level, raw) {
    var id = this.options.headerPrefix + raw.toLowerCase().replace(/[^\w]+/g, '-');
    return `<h${level} id="${id}">${text}</h${level}>\n`;
};

KostyaTretyak · 2018-01-09T14:54:16Z

As for me, marked is rather uncomfortable when it removes all the spaces for testing from resulting HTML files.

Feder1co5oave · 2018-01-09T14:56:17Z

No, I meant `<h${level}>${text}</h${level}>`, without any id at all.

Spaces are too tricky to make sure they match perfectly.

KostyaTretyak · 2018-01-09T15:02:59Z

I have rewritten test-script in myself:

#9. failed def_blocks.md

Expected:
~~~~~~~~> in /var/www/marked-ts/test/tests/def_blocks.html:25:4

'<p><a href="http://spec.commonmar'


Actual:
--------> in /var/www/marked-ts/test/tests/def_blocks-actual.html:25:4

'<p>[spec]</p>'


Excerpt tokens: [ { line: 24, type: 'blockquoteEnd' },
  { line: 25, type: 'paragraph', text: '[spec]' },
  { type: 'blockquoteStart' },
  { type: 'text',
    text: '[spec]: http://spec.commonmark.org/0.28/#example-181' },
  { line: 28, type: 'blockquoteEnd' } ]
links: { '4': { href: 'hello', title: undefined } }

joshbruce · 2018-01-09T15:15:12Z

Throwing copper...

I believe we're using a sort of homegrown solution for testing, yeah?

I don't see JSUnit, for example, as a dev dependency. Writing code to solve our specific problem is great, but might be nice for other developers to not have to learn some new way to write tests as well.

Feder1co5oave · 2018-01-09T15:17:25Z

I don't think unit testing is suitable for this. The current testing concept (input-vs-expected-output) is the right one, it just needs to be more homogeneous.

UziTech · 2018-01-09T16:03:45Z

I thought about moving to a modern testing framework (jasmine, mocha, etc.) when rewriting the tests but I agree with @Feder1co5oave. The input output approach is much easier than writing unit tests with fixtures.

We could add some sort of unit tests on top of the current test to provide more control. At the moment there is no way to run a test that changes the options in between tests without writing multiple tests.

joshbruce · 2018-01-10T01:00:05Z

Just to deal with the language thing to make sure I'm on the same page with y'all. I tend to lump tests into three groups: unit, integration, and application.

Unit: Testable in isolation of a single function or class to confirm input-output.
Integration: Requires other objects to confirm input-output.
Application: Tests how a user actually interacts with a system.

In my brain, we'd being doing more of an integration test - I'm with y'all - not concerned the parts in isolation (sorry if the JSUnit mention got us on the "unit" testing thing). Want to verify we get the desired output based on a specific input. Doesn't mean we can't use a more standard testing library see 8fold Component: https://github.com/8fold/php-html-component/blob/master/tests/ComponentTest.php

The only issue from a developer experience perspective, would be in the markdown versus HTML files. But, I defer to @UziTech more than anyone, because he seems the most familiar and I don't have the bandwidth to dive too deep on things. (Thanks again, y'all have been a pleasure to work with.)

All right. LGTM! Easy to change in future without affecting our users...rollback or something else. And confirmed. Again, awesome work. Thank you.

UziTech added 17 commits January 2, 2018 11:12

add fixed tests to /new

950e65e

change .text to .md

f31d3fa

remove invalid failing test check

5d2e796

add /test/tests to .gitignore

368952d

always "fix" tests before testing

3100519

change folder name to compiled_tests

951f2c3

use front-matter for test options

52bfc9c

fix bench

9e0ec53

linting

e5571be

add markdown-it to bench

03f510d

fix octal literal

26de4f1

update readme with new bench

253d975

update texting description

59846d9

clean up

b61af2a

Merge branch 'master' into test-fix

fd5d192

minify

7ee001a

clean up

2942655

joshbruce mentioned this pull request Jan 6, 2018

Added support to adding simple block rules #1003

Closed

KostyaTretyak reviewed Jan 6, 2018

View reviewed changes

UziTech mentioned this pull request Jan 6, 2018

Add an option to disable markdown elements #660

Closed

KostyaTretyak reviewed Jan 6, 2018

View reviewed changes

add error message on bench

33b8848

joshbruce merged commit ba26457 into markedjs:master Jan 6, 2018

UziTech mentioned this pull request Jan 7, 2018

sort tests #1004

Merged

UziTech deleted the test-fix branch April 5, 2018 14:38

This was referenced Mar 14, 2021

Bump marked from 0.3.6 to 0.3.19 datahjelpen/PEAK#7

Open

Bump marked from 0.3.6 to 0.3.19 Zemhart/project_UAS_PHP#10

Merged

Bump marked from 0.3.9 to 0.3.18 berez23/NodeGoat#2

Closed

snyk-bot mentioned this pull request Jul 12, 2022

[Snyk] Fix for 2 vulnerabilities Omrisnyk/goof-kubernetes#12

Open

snyk-bot mentioned this pull request Dec 29, 2022

[Snyk] Security upgrade marked from 0.3.5 to 0.4.0 Omrisnyk/goof-jenkins#22

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix tests #1002

Fix tests #1002

UziTech commented Jan 6, 2018 •

edited

joshbruce commented Jan 6, 2018 •

edited

KostyaTretyak Jan 6, 2018

UziTech Jan 6, 2018

KostyaTretyak Jan 6, 2018 •

edited

UziTech Jan 6, 2018

KostyaTretyak Jan 6, 2018

UziTech Jan 6, 2018

joshbruce Jan 7, 2018

UziTech Jan 7, 2018

UziTech commented Jan 6, 2018 •

edited

joshbruce commented Jan 6, 2018 •

edited

joshbruce commented Jan 7, 2018

Feder1co5oave commented Jan 9, 2018

KostyaTretyak commented Jan 9, 2018

KostyaTretyak commented Jan 9, 2018 •

edited

Feder1co5oave commented Jan 9, 2018

KostyaTretyak commented Jan 9, 2018 •

edited

joshbruce commented Jan 9, 2018

Feder1co5oave commented Jan 9, 2018

UziTech commented Jan 9, 2018

joshbruce commented Jan 10, 2018

Fix tests #1002

Fix tests #1002

Conversation

UziTech commented Jan 6, 2018 • edited

joshbruce commented Jan 6, 2018 • edited

KostyaTretyak Jan 6, 2018

Choose a reason for hiding this comment

UziTech Jan 6, 2018

Choose a reason for hiding this comment

KostyaTretyak Jan 6, 2018 • edited

Choose a reason for hiding this comment

UziTech Jan 6, 2018

Choose a reason for hiding this comment

KostyaTretyak Jan 6, 2018

Choose a reason for hiding this comment

UziTech Jan 6, 2018

Choose a reason for hiding this comment

joshbruce Jan 7, 2018

Choose a reason for hiding this comment

UziTech Jan 7, 2018

Choose a reason for hiding this comment

UziTech commented Jan 6, 2018 • edited

joshbruce commented Jan 6, 2018 • edited

joshbruce commented Jan 7, 2018

Feder1co5oave commented Jan 9, 2018

KostyaTretyak commented Jan 9, 2018

KostyaTretyak commented Jan 9, 2018 • edited

Feder1co5oave commented Jan 9, 2018

KostyaTretyak commented Jan 9, 2018 • edited

joshbruce commented Jan 9, 2018

Feder1co5oave commented Jan 9, 2018

UziTech commented Jan 9, 2018

joshbruce commented Jan 10, 2018

UziTech commented Jan 6, 2018 •

edited

joshbruce commented Jan 6, 2018 •

edited

KostyaTretyak Jan 6, 2018 •

edited

UziTech commented Jan 6, 2018 •

edited

joshbruce commented Jan 6, 2018 •

edited

KostyaTretyak commented Jan 9, 2018 •

edited

KostyaTretyak commented Jan 9, 2018 •

edited