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

Replace mustache_test with generated spec, and fix failures #43

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

adammck
Copy link

@adammck adammck commented Feb 23, 2014

This replaces the mustache_test file with a much more comprehensive test suite generated from the official Mustache spec. I wrote a little generator to do this rather than parsing the spec and generating the tests at runtime, because I think the output is surprisingly idiomatic and comprehensible.

This improved test suite causes about 50 failures, a runtime panic and an infinite loop, so you might want to look into those before merging. (Thankfully most of them are related to whitespace and error messages, so should be simple to fix.) I've already started working on those in my fork, and can update this PR when they're all fixed.

@hoisie
Copy link
Owner

hoisie commented Feb 24, 2014

That looks really, really cool. Looking forward to seeing progress on this. Are there features this spec tests that mustache.go doesn't have yet?

@adammck
Copy link
Author

adammck commented Mar 16, 2014

The failures are mostly deviations from the spec rather than missing features. After fixing the panic and the infinite loop, here are the failures, reformatted a bit for readability. Most of them are related to whitespace (which IMHO mustache makes way too complicated), but if we ignore those for now, here are 15 remaining failures. Mostly ampersand interpolation and dotted names.

adammck and others added 7 commits March 16, 2014 18:42
I thought it'd be clearer to embed the JSON straight from the spec, but
with all the escaping, it was actually pretty opaque. This is simpler.
The TestPartialsRecursion test currently causes an infinite loop,
because partials are being parsed eagerly, even when they will never be
rendered. This defers parsing until the partial needs to be rendered.
This fixes a runtime panic in the TestInvertedContextMisses, which
attempts to render an inverted block with an empty contextChain.
HTML entities inside {{&ampersand}} mustaches should not be escaped.
The var name was previously not being stripped.
Missing partials should render an empty string, not an error message.
@adammck adammck changed the title Replace mustache_test with generated spec Replace mustache_test with generated spec, and fix failures Mar 18, 2014
@adammck
Copy link
Author

adammck commented Mar 18, 2014

Update: I've fixed everything except for 32 whitespace-related failures. Those will be a bit tricky, since Mustache's whitespace rules are pretty weird. I think it would be useful to skip those failing tests for now and get this merged, since the output is otherwise to spec. What do you think?

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.

3 participants