Refactor test helpers #1336

ylemkimon · 2018-05-21T08:11:56Z

Combine common codes from parse and build, and dispatch using Mode
Remove toNotBuild and toNotParse and use expect.not
Support tagging literals: Refactor test helpers #1336 (comment)
Improve failed test message:
- Use color
- Print stack traces
- Print diff
- Follow jest matcher output style

expect	tests	success	ParseError	Warning	other errors
expect.toParse	parse	O	X	X	X
expect.not.toParse	parse	X	O	O	X
expect.toBuild	build	O	X	X	X
expect.not.toBuild	build	X	O	O	X
expect.toFailWithParseError	parse	X	O*	X	X
expect.not.toFailWithParseError	parse	O	X**	O	X
expect.toWarn	build	X	X	O	X
expect.not.toWarn	build	O	O	X	X

(O = pass, X = fail)
* only if matches given string, if given
** only if doesn't match given string, if given

* Combine common codes from `parse` and `build`, and dispatch using `Mode` * Remove `toNotBuild` and `toNotParse` and use `expect.not` * Remove `Warning` and instead mock `console.warn` * Add `expect.toHavePassed` and check whether parse/build succeeded lazily * Improve failed test `message`: - Use color - Print stack traces(excluding internals) - Print diff - Follow jest matcher output style

codecov · 2018-05-21T08:15:43Z

Codecov Report

❗ No coverage uploaded for pull request base (master@c994722). Click here to learn what that means.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master    #1336   +/-   ##
=========================================
  Coverage          ?   83.87%           
=========================================
  Files             ?       79           
  Lines             ?     4539           
  Branches          ?      797           
=========================================
  Hits              ?     3807           
  Misses            ?      639           
  Partials          ?       93

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c994722...c64ef13. Read the comment docs.

edemaine · 2018-05-21T15:06:04Z

The advantage of the previous global warning -> error conversion is that we could make sure no code produced warnings when it wasn't supposed to. In fact, I was going to suggest changing the default strict setting to the actual default, instead of strict: false, to make sure no warnings were generated when they weren't supposed to. I guess such warnings would be printed to console, but it wouldn't prevent a PR from merging, for example. What do you think?

getParsed throws an error if parsing fails.

kevinbarabash · 2018-05-28T03:39:49Z

@ylemkimon are you planning changes or should I review?

edemaine · 2018-05-31T20:10:05Z

@ylemkimon I'm no longer in agreement on the warnings. Running from the command-line, I'd see warnings, but if they don't cause tests to fail, Travis will still allow the merge, which it shouldn't. Real-life example: In #1385 discussion, we spotted a bug through the existing warning -> error mechanism that we may not have noticed otherwise.

kevinbarabash · 2018-06-05T01:08:37Z

@edemaine is there an action here for @ylemkimon? I'm not sure what the next step is for this PR.

edemaine · 2018-06-05T08:02:19Z

I would turn warnings back into errors, but that's open to discussion.
Someone needs to actually review the code; I haven't yet.
Resolve conflicts (sorry!).
@ylemkimon wanted to add some literal magic.

kevinbarabash · 2018-06-16T01:28:42Z

Since it sounds like there's still some changes pending I'm going to mark this as "needs-revision".

…into jest-test-helpers

* Remove compareParseTree

ylemkimon · 2018-06-19T00:46:34Z

This is ready for review.

kevinbarabash · 2018-06-27T13:02:41Z

I'm going to try to look at this this weekend. I should've suggested saving the tagged template change for a separate PR.

ylemkimon · 2018-06-27T17:02:18Z

@kevinbarabash I agree. For review:

Test helper improvement: helpers.js, setup.js
- b3dcf40 (except around L221)
- 4e84e93
- 4e387b8 (minor)
- f783eb8
Test cases update:
Tagged literals:
- 17ca384
- a8c91eb
Minor fixes(no semantic change):

ylemkimon · 2018-06-27T17:04:42Z

test/katex-spec.js

-        const parseB = stripPositions(getParsed("x_3^2"));
-
-        expect(parseA).toEqual(parseB);
+        expect`x^2_3`.toParseLike`x_3^2`;


Changed to use toParseLike. (b3dcf40)

ylemkimon · 2018-06-27T17:23:55Z

test/katex-spec.js

-        expect("\text{hello% comment\n\nworld}")
-            .toParseLike("\text{hello world}");
+        expect("\\text{hello% comment 1\nworld}").toParseLike`\text{helloworld}`;
+        expect("\\text{hello% comment\n\nworld}").toParseLike`\text{hello world}`;


There were some cases of using single backslash with t resulting in a horizontal tab(\t). (17ca384)

kevinbarabash · 2018-06-29T02:54:34Z

@ylemkimon thanks for group the commits into logical groupings. I'll look at the individual commits when I do this review.

kevinbarabash

@ylemkimon thanks for taking a stab at this. It needs some discussion.

kevinbarabash · 2018-07-07T23:34:53Z

test/errors-spec.js

@@ -6,76 +6,76 @@ describe("Parser:", function() {

    describe("#handleInfixNodes", function() {
        it("rejects repeated infix operators", function() {
-            expect("1\\over 2\\over 3").toFailWithParseError(
+            expect`1\over 2\over 3`.toFailWithParseError(


This looks a little strange to me b/c I'm not used to template strings returning things that aren't strings. Maybe we could introduce a tex template literal function. It would look like:

expect(tex`1\over 2\over 3`).toFailWithParseError(...

The tex function could be exported for use by other people. Also, it would be easier to understand what's going on. I'm still not sure how expect is acting as a template function without us modifying/replacing it with our own implementation.

@kevinbarabash I agree that it can be unfamiliar since it's new syntax. However, it's new way to call the function with arguments: an array of strings with raw property, an array of raw strings, and expressions in ${}. As expect accepts any object, it just passes the first argument through and we can handle them in the matchers. Maybe we can document them in the CONTRIBUTING.md?

If expect passes through only one argument, does that mean this won't work with ${interpolated expressions}?

I don't think the new code looks much more confusing than the old code -- the weirdest part is that expect just passes on its argument, and the action is in the chained methods afterward -- but that's a general Jest thing. However, until I read the template literal description I didn't really understand what the code was doing.

What about expectTex`1 \over 2 \over 3` which is equivalent to expect(tex`1 \over 2 \over 3`), i.e., the string gets processed before calling expect, instead of requiring all matches to process it? I think the weirdest part is overloading expect in this unintended way (unless Jest suggests doing this?).

@edemaine Yes, it will throw an error if there are ${interpolated expressions} and r will return the part before the first one.

I think we can think of expect.toParse as expect the raw string of the tagged literal to success parsing.

It's not related, but Jest provides a way of creating a table using tagged template literal:

describe.each` a | b | expected ${1} | ${1} | ${2} ${1} | ${2} | ${3} ${2} | ${1} | ${3} `('$a + $b', ({a, b, expected}) => { test(`returns ${expected}`, () => { expect(a + b).toBe(expected); }); test(`returned value not be greater than ${expected}`, () => { expect(a + b).not.toBeGreaterThan(expected); }); test(`returned value not be less than ${expected}`, () => { expect(a + b).not.toBeLessThan(expected); }); });

Interesting. I was unaware of jest's use of tagged template literals. expectTex seems like a good compromise.

@ylemkimon apologies for not raising my concerns about expect as a tagged template earlier.

@kevinbarabash @edemaine

@ylemkimon apologies for not raising my concerns about expect as a tagged template earlier.

I don't mind. I think productive discussion is necessary.

What about expectTex`1 \over 2 \over 3` which is equivalent to expect(tex`1 \over 2 \over 3`), i.e., the string gets processed before calling expect, instead of requiring all matches to process it? I think the weirdest part is overloading expect in this unintended way (unless Jest suggests doing this?).

Interesting. I was unaware of jest's use of tagged template literals. expectTex seems like a good compromise.

I still disagree. The purpose of using tagged template literal is to avoid using escape backslashes and simplify test cases. I think using expectTeX defeats this purpose as there is no difference between processing before passing to expect and processing in the matchers, when using KaTeX matchers.

@ylemkimon There would be a difference if we needed to use Jest's matchers. But I can't actually think of any situation where we'd want to use those on a TeX string.

kevinbarabash · 2018-07-07T23:39:17Z

test/helpers.js

+/**
+ * Return the first raw string if x is tagged literal. Otherwise return x.
+ */
+export const r = x => x != null && x.hasOwnProperty('raw') ? x.raw[0] : x;


This function needs a better name.

@kevinbarabash I borrowed it from python's raw string prefix. Maybe tex, as you suggested above?

I wonder if people will want to use it like

tex`\frac{${num}}{${den}}`

That could be a separate PR though if people think that's worth supporting.

@kevinbarabash Wouldn't it be same as String.raw?

I supposed it would be in which case I guess people can just use String.raw. 😅

kevinbarabash · 2018-07-07T23:45:23Z

test/helpers.js

    try {
-        return parseTree(expr, settings);
+        expect(result).toHavePassed();


I would expect getTree to call katex.render() and return the tree or clean up the stack trace and rethrow as necessary. Then expectKaTeX can call getTree and we don't have introduce a special matcher just to make things work in this order. Is there something I'm missing that necessitates this setup?

kevinbarabash · 2018-07-07T23:47:26Z

test/helpers.js

 };

-export const buildAndSetResult = function(expr, result,
-                                          settings = new Settings()) {
+export const expectKaTeX = (expr, settings = new Settings(), mode, isNot,


Does the default settings work when undefined is passed as the second arg?

I really like that we're supporting .not now, but this helper function has complicated the implementation quite a bit. I'd rather have a few small helper functions that each of the toParse(), toBuild(), etc. call then a single function that tries to do it all. Maybe we could add something format the message in addition to the function you added to clean up the stack trace.

@kevinbarabash

Does the default settings work when undefined is passed as the second arg?

Yes. Default function parameters allow formal parameters to be initialized with default values if no value or undefined is passed.

* Move `_getBuilt` into `getBuilt` * Move default settings and tagging literal support in to `getParsed` and `getBuilt` * Remove stack traces clean-up * Remove `toHavePassed` matcher

ylemkimon · 2018-07-08T05:43:02Z

@kevinbarabash

I would expect getTree to call katex.render() and return the tree or clean up the stack trace and rethrow as necessary. Then expectKaTeX can call getTree and we don't have introduce a special matcher just to make things work in this order.

Yes, I agree. My original intention was to reuse the error message from expectKaTeX and clean-up stack traces. However, now I think, they are not needed, they made codes unnecessarily complicated, and even made errors hard to debug.

I've removed getTree in f783eb8.

I really like that we're supporting .not now, but this helper function has complicated the implementation quite a bit. I'd rather have a few small helper functions that each of the toParse(), toBuild(), etc. call then a single function that tries to do it all. Maybe we could add something format the message in addition to the function you added to clean up the stack trace.

With removing getTree, I think expectKaTeX became simple enough. Maybe we can extract expected string construction into separate function, like printActualErrorMessage?

kevinbarabash · 2018-07-08T20:14:34Z

With removing getTree, I think expectKaTeX became simple enough. Maybe we can extract expected string construction into separate function, like printActualErrorMessage?

Sounds good.

ylemkimon · 2018-07-09T11:23:30Z

With removing getTree, I think expectKaTeX became simple enough. Maybe we can extract expected string construction into separate function, like printActualErrorMessage?

Sounds good.

Done in fc47230.

kevinbarabash

Sorry for letting this sit for so long. Let's get this merged.

ylemkimon added 2 commits May 21, 2018 19:27

Update helpers.js

6cdff3c

Merge branch 'master' into jest-test-helpers

d751297

This comment has been minimized.

Sign in to view

ylemkimon mentioned this pull request May 24, 2018

\def support (and \gdef and \global\def) #1348

Merged

Remove toBeTruthy checks

4250d06

getParsed throws an error if parsing fails.

ylemkimon changed the title ~~Refactor test helpers~~ [WIP]Refactor test helpers May 25, 2018

This comment has been minimized.

Sign in to view

kevinbarabash added the GH Review: review-needed label May 28, 2018

kevinbarabash changed the title ~~[WIP]Refactor test helpers~~ Refactor test helpers May 28, 2018

edemaine mentioned this pull request May 29, 2018

\newcommand, \renewcommand, \providecommand #1382

Merged

edemaine mentioned this pull request Jun 4, 2018

Automatic mrel/mbin spacing for \boldsymbol #1388

Merged

Merge branch 'master' into jest-test-helpers

7343e48

kevinbarabash added GH Review: needs-revision and removed GH Review: review-needed labels Jun 16, 2018

ylemkimon added 8 commits June 16, 2018 13:58

Merge branch 'master' into jest-test-helpers

0c96b20

Merge branch 'jest-test-helpers' of https://github.com/ylemkimon/KaTeX …

6e1773b

…into jest-test-helpers

Used tagged literals

17ca384

Use .toHaveLength

721f62f

Use tagged literals

a8c91eb

Use to{Build,Parse}Like where possible

96cdfa0

* Remove compareParseTree

Use snapshot where possible

4ad8dfd

Join into one line where <88 chars

aeb5350

ylemkimon added GH Review: review-needed and removed GH Review: needs-revision labels Jun 18, 2018

ylemkimon added 2 commits June 19, 2018 08:17

Remove call to console.warn from stack traces

4e387b8

Merge branch 'master' into jest-test-helpers

6f6865d

Merge branch 'master' into jest-test-helpers

d89600c

kevinbarabash self-assigned this Jun 27, 2018

ylemkimon commented Jun 27, 2018

View reviewed changes

Merge branch 'master' into jest-test-helpers

527c5b3

Merge branch 'master' into jest-test-helpers

92f13b1

kevinbarabash reviewed Jul 8, 2018

View reviewed changes

ylemkimon added 3 commits July 8, 2018 14:34

Fix merge errors

a49c067

Merge branch 'master' into jest-test-helpers

6572122

Remove getTree

f783eb8

* Move `_getBuilt` into `getBuilt` * Move default settings and tagging literal support in to `getParsed` and `getBuilt` * Remove stack traces clean-up * Remove `toHavePassed` matcher

Extract expected string construction into printExpectedResult

fc47230

edemaine mentioned this pull request Jul 11, 2018

Remove tree cloning before building HTML #1470

Merged

Merge branch 'master' into jest-test-helpers

b557735

kevinbarabash approved these changes Jul 22, 2018

View reviewed changes

Merge branch 'master' into jest-test-helpers

c64ef13

kevinbarabash merged commit 71035c7 into KaTeX:master Jul 22, 2018

ylemkimon deleted the jest-test-helpers branch August 10, 2018 10:43

ylemkimon mentioned this pull request Aug 18, 2018

Release v0.10.0-rc #1634

Merged

1 task

snyk-bot mentioned this pull request Feb 23, 2020

[Snyk] Upgrade katex from 0.9.0 to 0.11.1 saurabharch/Rocket.Chat#1

Open

Refactor test helpers #1336

Refactor test helpers #1336

Conversation

ylemkimon commented May 21, 2018 • edited

codecov bot commented May 21, 2018 • edited

Codecov Report

edemaine commented May 21, 2018

This comment has been minimized.

kevinbarabash commented May 28, 2018

This comment has been minimized.

edemaine commented May 31, 2018

kevinbarabash commented Jun 5, 2018

edemaine commented Jun 5, 2018

kevinbarabash commented Jun 16, 2018

ylemkimon commented Jun 19, 2018

kevinbarabash commented Jun 27, 2018

ylemkimon commented Jun 27, 2018 • edited

ylemkimon Jun 27, 2018 • edited

Choose a reason for hiding this comment

ylemkimon Jun 27, 2018 • edited

Choose a reason for hiding this comment

kevinbarabash commented Jun 29, 2018

kevinbarabash left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edemaine Jul 8, 2018 • edited

Choose a reason for hiding this comment

ylemkimon Jul 8, 2018 • edited

Choose a reason for hiding this comment

ylemkimon Jul 8, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ylemkimon Jul 9, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ylemkimon commented Jul 8, 2018 • edited

kevinbarabash commented Jul 8, 2018

ylemkimon commented Jul 9, 2018

kevinbarabash left a comment

Choose a reason for hiding this comment

ylemkimon commented May 21, 2018 •

edited

codecov bot commented May 21, 2018 •

edited

ylemkimon commented Jun 27, 2018 •

edited

ylemkimon Jun 27, 2018 •

edited

ylemkimon Jun 27, 2018 •

edited

edemaine Jul 8, 2018 •

edited

ylemkimon Jul 8, 2018 •

edited

ylemkimon Jul 8, 2018 •

edited

ylemkimon Jul 9, 2018 •

edited

ylemkimon commented Jul 8, 2018 •

edited