-
Notifications
You must be signed in to change notification settings - Fork 8
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.
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
Add complete unit tests for utils interface #5
Conversation
]; | ||
utils.appendChild(jml, fragment); | ||
assert.deepEqual(fragment, []); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added this test when I realized the fragment
was being muted (by calling child.shift()
). I wasn't sure if this was intentional. Personally, I would argue that functions should be pure whenever possible and that the child param should not be mutated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's what a fix would look like:
https://github.com/pgoldrbx/jsonml.js/compare/test/utils...pgoldrbx:fix/child-fragment-mutation?expand=1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, we had better to make function pure, and you can fix it in next PR, thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const child = new function RawChild() {}; | ||
utils.appendChild(jml, child); | ||
assert.strictEqual(jml, ['div', child]); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here is what I believe a fix would be:
https://github.com/pgoldrbx/jsonml.js/compare/test/utils...pgoldrbx:fix/raw-child?expand=1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just raise a new issue, I need to to read the source code, and may be re-design these APIs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const attrs = { a: 1 }; | ||
utils.appendChild(jml, attrs); | ||
assert.deepEqual(jml, ['div', { a: 1 }]); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This also fails but it is due to the error above. The isRaw
call happens before the method can reach this logic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just raise a new issue, I need to to read the source code, and may be re-design these APIs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also fixed by #10
|
||
utils.appendChild(jml, 'z'); | ||
assert.deepEqual(jml, ['', 'xz']); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added this test based on the condition to allow a parent element with no length. I'm not really sure what the purpose is. To build a fragment?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just raise a new issue, I need to to read the source code, and may be re-design these APIs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Filed issue: #11
This PR adds all missing test for the
utils
methods. I have tried to be strict while adhering to the existing test style as much as possible.NOTE: These tests raise at least one bug and some other outstanding questions.
isRaw
is called byappendChild
but is not defined.Failing tests are commented and skipped for the moment.