-
Notifications
You must be signed in to change notification settings - Fork 80
Runtime behavior tests #135
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
Conversation
a51498e
to
2e64d46
Compare
@@ -24,11 +24,13 @@ ifneq (,$(wildcard ./test/__setup.js)) | |||
@mocha --recursive --ui tdd \ | |||
--require babel-register \ | |||
--require babel-polyfill \ | |||
--require ./test/__setup | |||
--require ./test/__setup \ | |||
test/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was getting some strange unknown option --recursive
errors from mocha
without this.
val += ch; | ||
} | ||
|
||
if (ps.currentIs('\n')) { | ||
throw new ParseError('E0020'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I submitted projectfluent/fluent#83 to reflect this change in the spec.
while (this._index < this._length) { | ||
this.skipInlineWS(); | ||
|
||
if (this._source[this._index] === ')') { | ||
return args; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moving this here allows the last argument to be followed by a comma. I filed projectfluent/fluent#84 to allow this in the spec.
const [entries] = parse(source); | ||
const expectedEntries = JSON.parse(json); | ||
|
||
for (const [id, expected] of Object.entries(expectedEntries)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Iterating over expectedEntries
here means that the runtime parser could possibly parse more entries and we wouldn't learn about it from these tests. I'd like to fix this in a follow-up later on.
test(ftlfilename, function() { | ||
return Promise.all( | ||
test(ftlfilename, async function() { | ||
const [ftl, expected] = await Promise.all( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No functional changes here; just switching to async
.
This is PR consists of 3 commits: the first one adds the tests and the other two fix the differences in parsing between the runtime and the tooling parser. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks awesome! So happy to see the coverage for the runtime parser! :)
Partial port of projectfluent/fluent.js#135 affecting the tooling parser.
Partial port of projectfluent/fluent.js#135 affecting the tooling parser.
Partial port of projectfluent/fluent.js#135 affecting the tooling parser.
This doesn't quite add behavior tests to the runtime parser because the runtime parser doesn't produce the same errors as the tooling one. What we can do, however, is parse all fixtures in
fluent-syntax
and check if the runtime parser parsed the same messages (check if keys are present) and if so, if their interface is the same (is value present or missing? which attributes are defined?).