Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add tests for helpers #16

Merged
merged 2 commits into from
Dec 14, 2017
Merged

Add tests for helpers #16

merged 2 commits into from
Dec 14, 2017

Conversation

pgoldrbx
Copy link
Contributor

@pgoldrbx pgoldrbx commented Dec 11, 2017 •
edited
Loading

Fixes #4

---------------|----------|----------|----------|----------|----------------|
File           |  % Stmts | % Branch |  % Funcs |  % Lines |Uncovered Lines |
---------------|----------|----------|----------|----------|----------------|
All files      |      100 |      100 |      100 |      100 |                |
 XMLToReact.js |      100 |      100 |      100 |      100 |                |
 helpers.js    |      100 |      100 |      100 |      100 |                |
---------------|----------|----------|----------|----------|----------------|

pgoldrbx
pgoldrbx commented Dec 11, 2017
View reviewed changes
test/helpers.test.js
fatalError: throwError,
warning: throwError
});
parseXML = (xml) => parser.parseFromString(xml, 'text/xml');
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is more programmatic work that i like to have inside a test suite. maybe it should get its own tests? not sure

Copy link
Contributor

Choose a reason for hiding this comment

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

ah, agreed! it feels worth it to extract XML parsing into its own helper. created #18 to track this

pgoldrbx
pgoldrbx commented Dec 11, 2017
View reviewed changes
test/helpers.test.js Outdated
it('should return child nodes as an array', () => {
const parsed = parseXML('<MyDocument><A>foo</A><B>bar</B><C>baz</C></MyDocument>');
// expect(getChildren(firstChild)).to.be.an('array');
console.log(JSON.stringify(parsed, 2, 2));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

weird things were happening - i don't remember where i left off 😆
probably just PEBKAC at the time

pgoldrbx
pgoldrbx commented Dec 11, 2017
View reviewed changes
test/helpers.test.js Outdated

// @TODO fix this?
it('should throw an error if called with `null`', () => {
expect(() => validateConverters(null)).to.throw(Error);
Copy link
Contributor Author

@pgoldrbx pgoldrbx Dec 11, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

@taveras we might as well fix this? it's the result of typeof null // => Object

pgoldrbx
pgoldrbx commented Dec 11, 2017
View reviewed changes
test/helpers.test.js
foo: 'hello',
bar: '5',
baz: 'true'
});
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is an interesting question — whether we should attempt to coerce attribute values. for now, the answer is probably "no" but we might consider it as a feature later?

Copy link
Contributor

Choose a reason for hiding this comment

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

that feels helpful! I created #17 to track this

@pgoldrbx pgoldrbx self-assigned this Dec 13, 2017
@pgoldrbx pgoldrbx changed the title [wip] Add tests for helpers Add tests for helpers Dec 13, 2017
pgoldrbx
pgoldrbx commented Dec 13, 2017
View reviewed changes
src/helpers.js
@@ -115,4 +115,3 @@ export function visitNode(node, index, converters, data) {

return createElement(type, newProps, ...childElements);
};

Copy link
Contributor Author

Choose a reason for hiding this comment

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

my IDE trims excess newlines. 🤷‍♂️

Copy link
Contributor

Choose a reason for hiding this comment

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

hah, i blame these newlines on Vim

i don't feel strongly about this, but we can do whichever our final lint rules require

pgoldrbx
pgoldrbx commented Dec 13, 2017
View reviewed changes
test/helpers.test.js
@@ -0,0 +1,233 @@
import { expect } from 'chai';
import { DOMParser } from 'xmldom';
import { isValidElement } from 'react';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added react as a dev dependency for this

pgoldrbx
pgoldrbx commented Dec 13, 2017
View reviewed changes
src/helpers.js
@@ -10,7 +10,7 @@ const ERR_INVALID_CONVERTER = 'XMLToReact: Invalid value for converter map argum
* @private
*/
export function validateConverters(converters) {
if (typeof converters !== 'object') {
if (typeof converters !== 'object' || !converters) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@taveras added this missing falsy check to handle null

This was referenced Dec 14, 2017
Open
Closed
taveras
taveras approved these changes Dec 14, 2017
View reviewed changes
Copy link
Contributor

@taveras taveras left a comment

Choose a reason for hiding this comment

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

LGTM! left a few notes

test/helpers.test.js
fatalError: throwError,
warning: throwError
});
parseXML = (xml) => parser.parseFromString(xml, 'text/xml');
Copy link
Contributor

Choose a reason for hiding this comment

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

ah, agreed! it feels worth it to extract XML parsing into its own helper. created #18 to track this

test/helpers.test.js
const data = { foo: 'bar' }
const { firstChild } = parseXML('<a x="1" y="two">hello</a>');
visitNode(firstChild, 0, converters, data);
sinon.assert.calledWith(converters.a, { x: '1', y: 'two' }, data);
Copy link
Contributor

Choose a reason for hiding this comment

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

💯

test/helpers.test.js
const element = visitNode(firstChild, 0, converters);

expect(isValidElement(element)).to.be.true;
expect(element.type).to.equal('ul');
Copy link
Contributor

Choose a reason for hiding this comment

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

how do you feel about using enzyme for these tests?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I considered it but was concerned that it was overkill. I also considered rendering to markup. Ultimately I wanted to limit additional dependencies or complexity. Further, enzyme has adapters for multiple versions of React, but our library is meant to support any 14|15|16 (and we do)

Copy link
Contributor

Choose a reason for hiding this comment

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

that's fair -- i'll review our use of enzyme in our other tests

@taveras taveras merged commit b7bc6c3 into CondeNast:master Dec 14, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@taveras taveras taveras approved these changes

Assignees

@pgoldrbx pgoldrbx

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@pgoldrbx @taveras