Skip to content
This repository has been archived by the owner on Apr 25, 2018. It is now read-only.

Unit test ElementProxy and make related fixes - part one #104

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion interfaces/test.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ declare type Assert = {
}

declare function describe (description: string, fn: Function): void
declare function it (description: string, fn: Function): void
declare function it (description: string, fn: ?Function): void
declare function beforeEach (): void
declare function afterEach (): void
declare var assert: Assert
26 changes: 17 additions & 9 deletions src/ElementProxy.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,26 +12,31 @@ import {NodeProxy} from './types'
export class ElementProxy {
_node: HTMLElement;

// Expose these so they can be stubbed for unit test
_emitMount: Function;
_emitUnmount: Function;

constructor (node: HTMLElement) {
this._node = node
}

emitMount (fn: Function): void {
emitMount(this._node, fn)
this._emitMount(this._node, fn)
}

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)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also the signature should probably be (fn: ?Function): void. That's my fault.

Copy link
Contributor Author

@brandonpayton brandonpayton May 10, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Better to inject emitMount and emitUnmount as part of the argument to the constructor and then test that they're called.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@brandonpayton brandonpayton May 11, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

children (): HTMLCollection {
return this._node.children
childNodes (): NodeList {
return this._node.childNodes
}

replaceChild (childProxy: NodeProxy, index: number): void {
const node = this._node
const child = childProxy._node
const replaced = node.children[index]
const replaced = node.childNodes[index]

if (isDefined(replaced)) {
node.replaceChild(child, replaced)
Expand All @@ -43,7 +48,7 @@ export class ElementProxy {
insertChild (childProxy: NodeProxy, index: number): void {
const node = this._node
const child = childProxy._node
const before: Node = node.children[index]
const before: Node = node.childNodes[index]

if (isDefined(before)) {
node.insertBefore(child, before)
Expand Down Expand Up @@ -136,12 +141,15 @@ export class ElementProxy {
return new ElementProxy(node)
}

static querySelector (selector: string): ElementProxy {
const node: HTMLElement = document.querySelector(selector)
return new ElementProxy(node)
static querySelector (selector: string): ?ElementProxy {
const node: ?HTMLElement = document.querySelector(selector)
return node ? new ElementProxy(node) : null
}

static fromElement (node: HTMLElement): ElementProxy {
return new ElementProxy(node)
}
}

set(ElementProxy.prototype, `_emitMount`, emitMount)
set(ElementProxy.prototype, `_emitUnmount`, emitUnmount)
205 changes: 205 additions & 0 deletions src/__test__/ElementProxy.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,205 @@
/* @flow */

import document from 'global/document'
import sinon from 'sinon/pkg/sinon'
import {ElementProxy} from '../ElementProxy'
import {TextProxy} from '../TextProxy'

describe(`ElementProxy`, () => {
it(`creates an element node`, () => {
const createdTagName = `div`
const elementProxy = ElementProxy.createElement(createdTagName)

const node = elementProxy._node
assert.equal(node.nodeType, Node.ELEMENT_NODE)

const expectedTagName = createdTagName.toUpperCase()
assert.equal(node.tagName, expectedTagName)
})

it(`provides proxied querySelector access to its document`, () => {
const elementNode = document.createElement(`div`)
elementNode.className = `testClass`
document.body.appendChild(elementNode)

try {
const elementProxy = ElementProxy.querySelector(`.testClass`)
assert.ok(elementProxy instanceof ElementProxy)
assert.equal(elementProxy && elementProxy._node, elementNode)

const nonexistentElementProxy = ElementProxy.querySelector(`.nonexistentClass`)
assert.equal(nonexistentElementProxy, null)
} finally {
elementNode.parentNode.removeChild(elementNode)
}
})

it(`creates a proxy from an existing element`)

it(`provides facility to emit mount and unmount events`, () => {
const elementProxy = ElementProxy.createElement(`div`)

assert.ok(`emitMount` in elementProxy)
assert.ok(`emitUnmount` in elementProxy)

// Simply verify that _emitMount and _emitUnmount are called as expected,
// assuming the implementations are tested elsewhere.
const expectedNode = elementProxy._node
const expectedMountCallback = () => {}
const expectedUnmountCallback = () => {}

elementProxy._emitMount = sinon.stub()
elementProxy.emitMount(expectedMountCallback)
assert.ok(elementProxy._emitMount.calledWith(expectedNode, expectedMountCallback))

elementProxy._emitUnmount = sinon.stub()
elementProxy.emitUnmount(expectedUnmountCallback)
assert.ok(elementProxy._emitUnmount.calledWith(expectedNode, expectedUnmountCallback))
})

const createTestElement = () => ElementProxy.createElement(`div`)
const createTestTextNode = () => TextProxy.createTextNode(`some-text`)

it(`appends a child`, () => {
const testAppend = (createTestProxy, createTestProxy2) => {
const parentProxy = ElementProxy.createElement(`div`)
const childNodes = parentProxy.childNodes()

assert.equal(childNodes.length, 0)

const childProxy = createTestProxy()
parentProxy.insertChild(childProxy, childNodes.length)

assert.equal(childNodes.length, 1)
assert.equal(childNodes[0], childProxy._node)

const childProxy2 = createTestProxy2()
parentProxy.insertChild(childProxy2, childNodes.length)

assert.equal(childNodes.length, 2)
assert.equal(childNodes[1], childProxy2._node)
}

testAppend(createTestElement, createTestElement)
testAppend(createTestElement, createTestTextNode)
testAppend(createTestTextNode, createTestElement)
testAppend(createTestTextNode, createTestTextNode)
})

it(`inserts a child before another child`, () => {
const testInsertBefore = (createBeforeNode, createInsertedNode) => {
const parentProxy = ElementProxy.createElement(`div`)
const childNodes = parentProxy.childNodes()

const beforeNodeProxy = createBeforeNode()
parentProxy.insertChild(beforeNodeProxy, 0)

// Confirm assumptions
assert.equal(childNodes.length, 1)
assert.equal(childNodes[0], beforeNodeProxy._node)

const insertedNodeProxy = createInsertedNode()
parentProxy.insertChild(insertedNodeProxy, 0)

assert.equal(childNodes.length, 2)
assert.equal(childNodes[0], insertedNodeProxy._node)
assert.equal(childNodes[1], beforeNodeProxy._node)
}

testInsertBefore(createTestElement, createTestElement)
testInsertBefore(createTestElement, createTestTextNode)
testInsertBefore(createTestTextNode, createTestElement)
testInsertBefore(createTestTextNode, createTestTextNode)
})

it(`replaces a child`, () => {
const testReplaceChild = (createNodeProxy, createReplacementProxy) => {
const parentProxy = ElementProxy.createElement(`div`)
const childProxies = [createNodeProxy(), createNodeProxy(), createNodeProxy()]
const childNodes = parentProxy.childNodes()

childProxies.forEach((childProxy, i) => parentProxy.insertChild(childProxy, i))

// Confirm assumptions
assert.ok(childProxies.every((childProxy, i) => childProxy._node === childNodes[i]))

const replacementNode = createReplacementProxy()
parentProxy.replaceChild(replacementNode, 1)
const expectedChildProxies = [childProxies[0], replacementNode, childProxies[2]]
assert.ok(expectedChildProxies.every((expectedProxy, i) => expectedProxy._node === childNodes[i]))
}

testReplaceChild(createTestElement, createTestElement)
testReplaceChild(createTestElement, createTestTextNode)
testReplaceChild(createTestTextNode, createTestElement)
testReplaceChild(createTestTextNode, createTestTextNode)
})

it(`removes a child`, () => {
const testRemoveChild = (createTestProxy, createTestProxy2) => {
const parentProxy = ElementProxy.createElement(`div`)
const childNodes = parentProxy.childNodes()

const childProxy = createTestProxy()

parentProxy.insertChild(childProxy, childNodes.length)
assert.equal(childNodes.length, 1)
assert.equal(childNodes[0], childProxy._node)
parentProxy.removeChild(childProxy)
assert.equal(childNodes.length, 0)

const childProxy2 = createTestProxy2()

parentProxy.insertChild(childProxy, childNodes.length)
parentProxy.insertChild(childProxy2, childNodes.length)
assert.equal(childNodes.length, 2)

parentProxy.removeChild(childProxy)
assert.equal(childNodes.length, 1)
assert.equal(childNodes[0], childProxy2._node)

parentProxy.removeChild(childProxy2)
assert.equal(childNodes.length, 0)

parentProxy.insertChild(childProxy, childNodes.length)
parentProxy.insertChild(childProxy2, childNodes.length)
assert.equal(childNodes.length, 2)

parentProxy.removeChild(childProxy2)
assert.equal(childNodes.length, 1)
assert.equal(childNodes[0], childProxy._node)

parentProxy.removeChild(childProxy)
assert.equal(childNodes.length, 0)
}

testRemoveChild(createTestElement, createTestElement)
testRemoveChild(createTestElement, createTestTextNode)
testRemoveChild(createTestTextNode, createTestElement)
testRemoveChild(createTestTextNode, createTestTextNode)
})

it(`provides access to the child nodes of its DOM node`, () => {
const parentNode = document.createElement(`div`)
const expectedChildNodes = [
document.createElement(`a`),
document.createElement(`p`),
document.createTextNode(`nested-text`),
document.createElement(`span`),
]

expectedChildNodes.forEach(childNode => parentNode.appendChild(childNode))

const elementProxy = new ElementProxy(parentNode)
const actualChildNodes = elementProxy.childNodes()

assert.equal(actualChildNodes.length, expectedChildNodes.length)
expectedChildNodes.forEach((expectedChildNode, i) => {
assert.equal(actualChildNodes[i], expectedChildNode)
})
})

it(`sets, gets, and removes attributes according to descriptors`)
it(`sets, gets, and removes properties according to descriptors`)
it(`sets, gets, and removes event listeners according to descriptors`)
})