Unit test ElementProxy and make related fixes - part one #104
Conversation
|
@brandonpayton Can you test this against #103 's test file |
|
Hi @jadbox, I tried and still saw the failures, which probably makes sense since the |
|
@garbles, I just noticed that you're using sinon for stubs elsewhere and will update this PR to use sinon as well. |
In addition to the tests, other changes are: * Updated `ElementProxy.querySelector` to return `null` when no matching element is found * Fixed `ElementProxy#children()` to be `ElementProxy#childNodes` to support text nodes as children * Added `_emitMount` and `_emitUnmount` methods to be stubbed by unit test
brandonpayton
force-pushed
the
add-elementproxy-unit-tests
branch
from
60c89c3
to
d6a1111
Compare
|
I updated the stubbed ElementProxy tests to use sinon. I thought I should go ahead and declare remaining ElementProxy tests with Thanks! |
| } | ||
|
|
||
| emitUnmount (fn: Function): void { | ||
| emitUnmount(this._node, fn) | ||
| // Rely on prototype method so it can be stubbed for unit test | ||
| this._emitUnmount(this._node, fn) |
There was a problem hiding this comment.
Why do this? If fn is falsy, then the mount/unmount event won't fire. I'm not a huge fan of stubbing out another module this way.
There was a problem hiding this comment.
Also the signature should probably be (fn: ?Function): void. That's my fault.
There was a problem hiding this comment.
It was an attempt at letting the unit tests verify that ElementProxy is interacting with an underlying API so that we don't write the same tests for ElementProxy as we have for mountable. I think it is weak, but it is the best I came up with. What would you do?
There was a problem hiding this comment.
Better to inject emitMount and emitUnmount as part of the argument to the constructor and then test that they're called.
There was a problem hiding this comment.
Would all uses of ElementProxy be required to provide implementation for emitMount and emitUnmount? If so, that seems overly burdensome given that it is implementation detail that is not likely to be changed except for unit test.
How would references to the injected implementations be maintained? Currently, I imagine it would be the same as this PR but injected via constructor rather than defaults set on the prototype. I may need another clue because my imagination is mostly producing what is here.
There was a problem hiding this comment.
Yea I guess I just don't see the point of calling a function to make sure that another function is called when that function is just a stub. You might as well stub emitMount instead of _emitMount
There was a problem hiding this comment.
Yeah, it seems a little silly. I was hoping to find a way to declare in the tests that ElementProxy needs to provide emitMount and emitUnmount while not unnecessarily testing what we're already testing for mountable.
There was a problem hiding this comment.
After a little more thought, I think it makes sense to test mount events with ElementProxy since ElementProxy is responsible for both adding the listeners and emitting the events. This is in contrast to mountable which is just responsible for emitting. My plan is to back out _emitMount and _emitUnmount and declare those tests to be added in a follow-up PR.
Sound OK?
There was a problem hiding this comment.
To shed a little more light on my original thinking, which I'm dropping anyway, the thought for stubbing _emitMount was to declare that ElementProxy relies on something else for emitting and interacts with it according to the call signature. But I don't think it buys anything but a sort of legal satisfaction.
|
I'm closing this and submitting a more limited PR with separate commits for tests and the fixes that satisfy them. |
Hi @garbles,
This is a PR for part one of the ElementProxy unit tests. We still need tests for
ElementProxy.fromElementand for set, get, and remove attributes. I'm working on those but am submitting this now with an ElementProxy fix. Insertion, replacement, and removal were broken with regard to text children. The fix is delivered with the unit test commit, which isn't ideal, but was made while writing the tests. Please let me know if you'd like it split into separate commits.Here's a summary of my changes:
ElementProxyunit testsElementProxy.querySelectorto returnnullwhen no matching element is foundElementProxy#children()toElementProxy#childNodesand switched the implementation to use the underlying element'schildNodesproperty rather thanchildrenbecausechildrencontains only elements which breaks insert, replace, and remove child operations when working with text nodes._emitMountand_emitUnmountmethods to be stubbed by unit test so we can test how they are called byElementProxy#emitMountandElementProxy#emitUnmountwhile relying on themountableunit tests to test the actual behavior. I don't love this, but it seems like a reasonable compromise.One question for part two:
new ElementProxy(domNode)andElementProxy.fromElement? I considered removingfromElementbut wanted to ask you first.