Screenshotter: Add --diff and --attempts options.

[kohler]

--diff produces a texcmp-like diff in test/screenshotter/diffs.

[kohler]

@gagern ?

[gagern]

Sorry, forgot to comment. I like the general idea.

I'm a bit confused by the fact that you are using a different convert invocation than texcmp. If your command is superior (looks like your green might be better to see), it would make sense to use that in both places for the sake of consistency.

As convert is invoked outside the docker, it might be worth indicating that ImageMagick has to be installed. Do we want to support gm convert from GraphicsMagick as an alternative? If so, how do we tell which one to use? Autodetect or let the user configure?

The line where you ignore errors from mkdir might be worth a comment. I assume that this is mainly in order to deal with EEXIST, relying on the fact that any other error creating the directory should lead to a write error later on. Correct?

I have a very slight dislike for the temporary file you write then delete. ImageMagick should be able to read that image from standard input. But adjusting the whole machinery so that it can feed input to the executed file makes things considerably more complicated and less generic, so I doubt that's actually worth changing.

[gagern]

Oh, and we probably should make sure the directory for the diffs is included in .gitignore. Right now that contains the singular diff for use by texcmp, while you are using plural diffs here. I find the distinction between singular and plural rather confusing, so I'd say either re-use diff or pick something more descriptive. I'd prefer reusing.

[kohler]

Sorry, forgot to comment. I like the general idea.

Good!

I'm a bit confused by the fact that you are using a different convert invocation than texcmp. If your command is superior (looks like your green might be better to see), it would make sense to use that in both places for the sake of consistency.

The texcmp invocation does not work for my imagemagick. (Version: ImageMagick 7.0.6-0 Q16 x86_64 2017-06-12 http://www.imagemagick.org)

As convert is invoked outside the docker, it might be worth indicating that ImageMagick has to be installed. Do we want to support gm convert from GraphicsMagick as an alternative? If so, how do we tell which one to use? Autodetect or let the user configure?

This is not a feature that's required to build or use katex so I do not want to do a ton of unnecessary hardening…

The line where you ignore errors from mkdir might be worth a comment. I assume that this is mainly in order to deal with EEXIST, relying on the fact that any other error creating the directory should lead to a write error later on. Correct?

Will comment.

I have a very slight dislike for the temporary file you write then delete. ImageMagick should be able to read that image from standard input. But adjusting the whole machinery so that it can feed input to the executed file makes things considerably more complicated and less generic, so I doubt that's actually worth changing.

Agree.

Oh, and we probably should make sure the directory for the diffs is included in .gitignore. Right now that contains the singular diff for use by texcmp, while you are using plural diffs here. I find the distinction between singular and plural rather confusing, so I'd say either re-use diff or pick something more descriptive. I'd prefer reusing.

Sorry, yes, diff is better.

[kohler]

Changed to diff.

[kohler]

@gagern any more comments? I have not changed texcmp imagemagick invocation but could do so later.

[edemaine]

Is this supposed to eventually replaced texcmp? Is the idea that screenshotter.sh --diff is simpler and possibly faster than screenshotter.sh then texcmp.sh (especially with changing directories in between)? I also like not having to remember two different ways to specify a single example to run.

I see that texcmp wasn't mentioned in the documentation anywhere. Maybe we should add some documentation to CONTRIBUTING.md (around Screenshot tests) about how to use this and/or texcmp? This would be a(nother) good place to mention "...if you have ImageMagick installed" for screenshotter.sh --diff.

[kohler]

texcmp and screenshotter.sh --diff have different purposes. Texcmp compares with tex output, screenshotter compares a new KaTeX-generated image with an older one.

[edemaine]

Oh, I see! That makes sense. But I didn't guess from the initial description the option help string. Perhaps change "produce image diffs" with "produce image diffs against existing screenshots" or similar?

[kohler]

????? was --verify reference in the previous help line not enough?

[edemaine]

It's clear now.

[kevinbarabash]

Hmm... not sure why travis didn't run tests on b3aa1b4.

[kevinbarabash]

@kohler can you push a commit with a trivial change? Hopefully that'll be enough to make travis run the tests.

[kevinbarabash]

LGTM

[kohler]

There's something weird going on with Travis. I rebased; here's a direct link to the successful Travis build, though that's not appearing here.

My repo: https://travis-ci.org/kohler/KaTeX/builds/250744199

PR: https://travis-ci.org/Khan/KaTeX/builds/250744209

[kevinbarabash]

Good enough for me.

[kevinbarabash]

@kohler thanks for the PR. Hopefully this is a one off situation with travis.