search

i-like-robots / rewireify

Rewireify is a port of Rewire for Browserify that adds setter and getter methods to each module so that their behaviour can be modified for better unit testing.
https://www.npmjs.com/package/rewireify
Other
59 stars 10 forks source link

Move getter and setter require statements to post so that test error line numbers are not skewed #2

Closed iamrandys closed 10 years ago

iamrandys commented 10 years ago

When developing unit tests, I found that the reported error line numbers were consistently off by 3 lines. I was able to move the fore code to the post to correct this issue.

i-like-robots commented 10 years ago

Thanks for your contribution, I don't see any harm in making this change (putting requirements at the top is only because I thought it was in some way 'neater') but I am a bit confused why it might be necessary... are the line numbers you're referring to from source maps generated before the rewireify transformation?

iamrandys commented 10 years ago

Yes, I agree. I like to have all of my require statements at the top as well. In my test environment, I never even see the source code and usability is more important in this case.

Here is an example karma error stack trace. I was having to subtract 3 lines from the stack trace to fine where the actual error occurred. It took me a while to finally figure out what was going on.

Thanks for creating such a useful tool!

Error: Expected spy error to have been called with [ 'SEARCH REQUEST FAILED', 'SearchActions', { storeName : 'TEST STORE NAME', error : '' } ] but actual calls were [ 'SEARCH REQUEST FAILED', 'SearchActions', { storeName : 'TEST STORE NAME', error : 'TEST ERROR' } ].
    at /var/folders/bp/gb8knv2n6sj1gt8mnsqwpckr0000gn/T/e71ef582adaec8926a36a5cce6c7b9892aafd3ba.browserify:4308:0 <- /Users/iamrandys/work/ui-app/lib-core/search/__test__/search-actions-test.js:75:0
    at /Users/iamrandys/work/ui-app/node_modules/karma-jasmine/lib/boot.js:113
    at /Users/iamrandys/work/ui-app/node_modules/karma-jasmine/lib/adapter.js:171
    at http://localhost:9876/karma.js:189
    at http://localhost:9876/context.html:213
i-like-robots commented 10 years ago

Did you ever get any more issues with this? I'm happy to merge the change if still required.

iamrandys commented 10 years ago

Yes, the line numbers are still off when we run the tests. My team has gotten use to subtracting 3 lines from the error messages, so they will be surprised when it gets fixed. It would be really nice if you'd merge it in.

On Sep 24, 2014, at 8:26 AM, Matt Hinchliffe notifications@github.com wrote:

Did you ever get any more issues with this? I'm happy to merge the change if still required.

— Reply to this email directly or view it on GitHub.

iamrandys commented 10 years ago

Works great now! Thanks @i-like-robots !