Update README and tests with comments from Juicy/imported-template/pull/31 #32
Conversation
…ll/31 - reset mock server, - don't use mock server if mockable by static files, - add pre-test control expectation to ensure conditions, - few typographical changes.
| @@ -37,6 +37,10 @@ | |||
| }); | |||
|
|
|||
| it('should remove stamped nodes after content is cleared', function(){ | |||
| var parent = myEl.parentNode; | |||
| // pre-test conditions | |||
| expect(parent.children.length).to.be.equal(2); | |||
There was a problem hiding this comment.
Can expect for preconditions be in beforeEach?
There was a problem hiding this comment.
Do you think it's a good practice? I've never seen such a use - assertion in setup? Aren't we going to write tests for tests?
There was a problem hiding this comment.
I've never seen testing for pre-test conditions in a test either. The fact that the test has to check preconditions looks like a design flaw in the testing framework. Maybe we can do some resetting of the parent container in beforeEach that makes this check redundant?
There was a problem hiding this comment.
We do have such resetting at
@alshakero as this was your idea to test preconditions could you elaborate?
There was a problem hiding this comment.
I'm not a testing expert. But when I see a test that is prone to fool me into a passing test, I try to test before and after the action in question.
For example, if we want to test delete:
expect(myObj.myVar).toBe(undefined)... is very likely to pass even if delete is broken. That's why I usually do:
expect(myObj.myVar).not.toBe(undefined);
delete myObj.myVar;
expect(myObj.myVar).toBe(undefined)And the tests I thought needed a control check are the ones that test for null and children.length == 1. Checking if children.count == 2 then remove(child) then children.count == 1 sounds like a more robust test than only children.count == 1.
However, I'm not asserting and merely suggesting.
There was a problem hiding this comment.
Well beforeEach makes sure we have elements there
Doing expect is just double checking. The question is whether it is a confusing paranoid mode, or better safe than sorry approach, as beforeEach uses <test-fixture> which may also have bugs.
There was a problem hiding this comment.
You have convinced me, let's keep it :)
There was a problem hiding this comment.
I wasn't 100% convinced either. I'm fine to keep it, but I'd like to avoid going to paranoid, to make tests easier to read and write.
| @@ -17,7 +17,7 @@ | |||
| <test-fixture id="juicy-html-with-content"> | |||
| <template> | |||
| <!-- nest to workaround test-fixture bug --> | |||
| <div><template is="juicy-html" content="/mock/smth"> | |||
| <div><template is="juicy-html" content="../mock/smth.html"> | |||
There was a problem hiding this comment.
Here you use relative path ../, but in test/content-legacy/no-content.html you use an absolute path /. This should be consistent.
test/content-legacy/no-content.html
Outdated
| //whenever the this returns true the request will not faked | ||
| return !url.match(/mock\//); | ||
| //whenever this returns true the request will not faked | ||
| return !url.match(/mock\/204/); | ||
| }); | ||
| server.respondWith('GET', '/mock/204', [204, {"Content-Type": "text/html"}, '']); |
There was a problem hiding this comment.
Maybe change the responses generated by sinon to be /generate/204 instead of /mock/204? This will make it easier to distinguish.
Or rename the mock directory to static.
make mock paths more consistent
for href tests with no content
|
BTW, once we will agree on all the changes, I'll squash commits, not to pollute commit log with Travis related problems. |
|
Might be hidden by GH #32 (comment) |
Juicy/imported-template/pull/31