Auto acquisition for parser result changes #130
Conversation
saschanaz
changed the title
test/invalid.js
Outdated
| expect(error.message).toEqual(err.message); | ||
| expect(error.line).toEqual(err.line); | ||
| } | ||
| for (const test of collect("invalid", true)) { |
There was a problem hiding this comment.
Nit: can you add a comment saying what the boolean trap is there? like:
collect("invalid", /*whatThisIs=*/ true)There was a problem hiding this comment.
Actually, let's fix this at the source.
marcoscaceres
left a comment
marcoscaceres
left a comment
There was a problem hiding this comment.
Some nits... only really ask that you avoid the boolean trap, but a few other suggestions to consider.
test/invalid.js
Outdated
| expect(error.message).toEqual(err.message); | ||
| expect(error.line).toEqual(err.line); | ||
| } | ||
| for (const test of collect("invalid", true)) { |
There was a problem hiding this comment.
Actually, let's fix this at the source.
test/util/collect.js
Outdated
| * @param {string} base | ||
| * @param {boolean} [expectError] | ||
| */ | ||
| function* collect(base, expectError) { |
There was a problem hiding this comment.
Let's make expectError not be a boolean. Maybe make it an object, so to avoid the boolean trap?
test/util/collect.js
Outdated
| * @param {string} path | ||
| * @param {Error} [error] | ||
| */ | ||
| function createItem(ast, path, error) { |
There was a problem hiding this comment.
Could be a class TestItem maybe? Might make this a little cleaner.
test/util/collect.js
Outdated
| ast, | ||
| path, | ||
| error, | ||
| jsonPath() { |
There was a problem hiding this comment.
nit: maybe get jsonPath()? then just item.jsonPath;
test/util/collect.js
Outdated
| jsonPath() { | ||
| return pth.join(pth.dirname(this.path), "../json", pth.basename(this.path).replace(".widl", ".json")); | ||
| }, | ||
| diff() { |
There was a problem hiding this comment.
Nit: I guess if we are only using this with testing it doesn't matter too much about using readFileSync... or could we spin up the tests to run in parallel?
|
Great reviews! I applied your feedbacks, although I'm not sure I understand this comment:
The |
I think even Node 6 supported promises, but obviously not |
I tried fixing #93 and got a BIG
ASTparser result changes, so I created a tool to automate change acquisitions.Much shorter test scripts as a bonus!
Also I fixed an issue that
typedef-uniontest was being ignored because of its file extension.