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

[testing] Use bem/html-differ to perform tests #1017

Closed
wants to merge 3 commits into from

Conversation

Feder1co5oave
Copy link
Contributor

I never liked the strategy of stripping all whitespace before comparing generated html code against the expected one. Taking into account whitespace when testing is important as it is often meaningful and leads to different results in the browser. For example, #1013 and #1011 fix issues that consist in different whitespace in the output, so even if tests are added they are not checked.

So, I thought we needed a more html-aware comparison algorithm that takes (meaningful) space into account. html-differ seemed to do the trick. It uses parse5 under the hood, which looks stable enough.

Also, the current text comparison algorithm is broken: whenever the expected html is shorter than the generated one, and matches everything up until its end, the test is marked as complete, even though marked outputted something more.

For more details about the proposed comparison algorithm, head over here. As an option, html-differ allows to ignore whitespace differences, html comments, and certain node attributes. I was thinking about taking advantage of this last feature to ignore headings' id attribute. Currently, header tags that do not have an id are "fixed" by the preprocessing phase of the test system, and ids are inserted into the expected html code automatically. This allows to test headings' ids when necessary, by specifying them in the .html file of the test case. (I used this in #456)

As an added benefit, when tests fail, we get a nice colored diff between the expected and the actual output, with space included, which wasn't before. As an example:

screenshot from 2018-01-18 01-39-26

I took the opportunity to lint the test/index.js source file and remove unused variables and so on.

Since html-differ treats space and newlines differently (I'm not sure this is in accordance to Html/CSS specs, see my inquiry at bem/html-differ#150), in order to make the main test case pass, I had to replace some simple spaces with newlines, when newlines were to be expected.
The uppercase_hex test case was plain wrong as it lacked some space which should have been there but was not required by the current comparison algorithm.

As addendum, I myself am not enthusiastic about yet another npm package dependency pulled in at startup. Including parse5 and a bunch of other packages for mere testing may seem overkilling. But the fact is that testing is the most important development phase of marked, especially if our goal is to adhere to commonmark as much as possible (their current test suite includes 624 test cases) and I think it needs to be done correctly. Comparing two different html codes and determining if they are equivalent is no easy task. It involves parsing html and checking the AST according to some very verbose specification that we cannot take care of all by ourselves.
Stripping all whitespace was a quick&easy solution to allow some testing, now's the time to step away from it.

/cc to @UziTech who was the kind author of the recent upgrade to the test system and may know it very well.

@UziTech
Copy link
Member

UziTech commented Jan 18, 2018

I like it. much easier to read. 💯

@KostyaTretyak
Copy link
Contributor

KostyaTretyak commented Jan 19, 2018

I also did not like stripping all whitespace in HTML. But as for me it is more clearly visible and simpler - just create a html file that have diff between expected and actual. After that, simply point to, for example,

Expected in test/tests/def_blocks.html:3:1
Actual in test/tests/def_blocks-actual.html:3:1

My editor allows me (with the Ctrl key pressed) to go through these links exactly to the place where there is a difference. In our case, the link points to the third line and the first character. I can see very clearly where the differences begin, I do not have to further distinguish between color text etc...

@Feder1co5oave
Copy link
Contributor Author

@KostyaTretyak so what would you do differently to test for whitespace?

@KostyaTretyak
Copy link
Contributor

In my fork marked-ts I do not stripping whitespaces and do a check expected to actual on each line.

@Feder1co5oave Feder1co5oave changed the title Use bem/html-differ to perform tests [testing] Use bem/html-differ to perform tests Jan 20, 2018
@joshbruce joshbruce added this to the 0.5.0 - Architecture and extensibility milestone Jan 20, 2018
@joshbruce
Copy link
Member

Right on! I'm going to flag this for 0.5.0; however, between this and linting - we may want to include both in the next release in general. Still catching up with the action over the last week. :)

@Feder1co5oave
Copy link
Contributor Author

Sounds good to me, it is no priority, even though this should help us test correctly.

I'd also like to take a look at a simpler solution: just normalizing html code on both sides, if I find the right library to do it.

@joshbruce
Copy link
Member

Think it might be worthwhile to find fallbacks to any libraries we include...just in case they end in the same position Marked was in before we all stepped back up, you know? Maybe a DEPENDENCIES.md file??

@joshbruce
Copy link
Member

See #1024

@joshbruce joshbruce removed this from the 0.5.0 - Architecture and extensibility milestone Apr 4, 2018
@joshbruce
Copy link
Member

Closing for now in favor of html-differ inclusion in #1160, merge conflicts, and possible future considerations related to developer operations and whatnot.

@joshbruce joshbruce closed this Apr 5, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants