Convert mocking to mockable imports (3/4) #1086
Conversation
src/annotator/guest.coffee
Outdated
| @@ -37,7 +38,7 @@ module.exports = class Guest extends Delegator | |||
| TextSelection: {} | |||
|
|
|||
| # Anchoring module | |||
| anchoring: require('./anchoring/html') | |||
| anchoring: htmlAnchoring | |||
There was a problem hiding this comment.
The new approach to mocking only currently supports dependencies declared at the top level of the module, so this hoists the require to the top of the file. This is needed anyway if we later convert this module to JS with import and export statements, since import can only appear at the top level as well.
Codecov Report
@@ Coverage Diff @@
## master #1086 +/- ##
==========================================
+ Coverage 92.25% 92.26% +<.01%
==========================================
Files 156 156
Lines 6136 6138 +2
Branches 975 975
==========================================
+ Hits 5661 5663 +2
Misses 475 475
Continue to review full report at Codecov.
|
yarn.lock
Outdated
| integrity sha512-KqKIFsPDQcXf2XZZazAxGa7xUgbGueLfCQb4mD7G16lvLYfz13SakC6IAcRaBueXwNn2O0K6VGlXZzPs0aLL3Q== | ||
| version "1.3.0" | ||
| resolved "https://registry.yarnpkg.com/babel-plugin-mockable-imports/-/babel-plugin-mockable-imports-1.3.0.tgz#29183a735fc3a27395b15b519e059fba7f61fde4" | ||
| integrity sha512-1kUED0ZXOLMKh1P90zHX9juD6IRWXgMB5yPtdupsaTJ9lKueAdGmRsuMnKXOlqtyaMYCW6oKwSz/gFLpiXaONA== | ||
|
|
There was a problem hiding this comment.
This update resolves an issue with the way CoffeeScript compiles groups of <name> = require(<module>) statements.
| } | ||
| htmlAnchoring = { | ||
| anchor: sinon.stub() | ||
| } | ||
|
|
There was a problem hiding this comment.
This ensures that the mocks for the highlighter and HTML anchoring modules are initialized for each test, for consistency with how we set up mocks in other tests.
Convert proxyquire mocking of several CoffeeScript modules.
Note that the exports of the `anchoring/html` module are captured when the class is constructed, so cannot be mocked later via `$imports`. Instead the created `Guest` object has to be monkey-patched.
robertknight
force-pushed
the
mockable-imports-part-3
branch
from
f32ac48 to
403bbda
Compare
src/annotator/test/guest-test.coffee
Outdated
|
|
||
| createGuest = (config={}) -> | ||
| config = Object.assign({}, guestConfig, config) | ||
| element = document.createElement('div') | ||
| return new Guest(element, config) | ||
| guest = new Guest(element, config) | ||
| guest.anchoring = htmlAnchoring |
There was a problem hiding this comment.
Why is this here instead of in the mocked imports? This feels hacky to me.
There was a problem hiding this comment.
This is necessary because guest.anchoring is initialized when the module is first evaluated, and so isn't affected by mocking that happens later. That's one limitation of this approach to mocking. See this node in the mocking plugin's README. The same issue applies to patch in Python.
An alternative solution would be to use constructor injection. Something conceptually like this:
class Guest(element, config, anchoring = htmlAnchoring) {
this.anchoring = anchoring
}With this approach, the default initialization expression is evaluated when the Guest constructor is called, so mocking would work.
There was a problem hiding this comment.
I've pushed a change which does this to handle the issue in a cleaner way ^
This enables mocking of the `anchoring/html` module in tests without having to patch the `anchoring` property after the class is constructed.
This is a follow-up to #1061 which converts remaining uses of proxyquire in .coffee files to use $imports.$mock instead - see earlier PR for a detailed explanation.
After this PR and #1085 are merged, the final PR will remove proxyquire as a dependency.