Rework hook error tests to actually assert #1781
Conversation
Yeah, this is a common issue. One way around it is to separate things out into two commits, one with a |
|
🎉 this commit is awesome! |
glenjamin
force-pushed
the
test-hook-errors
branch
from
7b10365 to
092ea46
Compare
|
I've split the move and the rewrite into separate commits, that's much neater. Also related to #1528 |
| var path = resolveFixturePath(fixture.name + '.js' || 'tempfile.js'); | ||
| args = args || []; | ||
|
|
||
| var fixtureContent = 'var fn = ' + fixture.toString() + '; fn()'; |
There was a problem hiding this comment.
That's pretty damn (beautifully) hacky ;)
Agree with both statements here. Definitely worth it though! 😄 |
Rework hook error tests to actually assert
The current
hook.errtests don't actually assert anything, to check that they work you have to uncomment a bunch of lines, and then see if the failing tests are the ones which are supposed to fail.In particular, this makes it difficult to safely implement #1635
I have converted these tests to a new approach which means they do now actually assert things.
I originally began using the
runMochahelper along with fixtures to do this, but I found that the tests were extremely opaque as they asserted on the details of a complex out-of-band fixture. To combat this I've done something a bit wacky - defined the fixture inline as a function, and then usedtoStringto send that function over the filesystem boundary to generate a fixture on the fly.I think the implementation wackiness is worth the expressive benefit here, but I wanted to get some feedback before I add any more tests for the new functionality described in #1635
As mentioned in the commit message, git isn't smart enough to detect the rewrite of this file, but you can force it to see that the diff doesn't change what is being asserted in any way.