add: some unit tests#315
add: some unit tests#315peterramsing merged 6 commits intopeterramsing:masterfrom wbruno:improve-unit-tests
Conversation
|
Woah. This is fun! ...I should probably use more "help wanted" tags instead of assuming nobody wants to help. 👍 I find there is a ton of work that goes into just managing a project like this by the time I have time to get to things like this I just want to go to bed. I need to get working at the "paid gig" but will check in on this at lunch. My wife and I have guests coming over tonight but will get some LostGrid time in after that late this evening (PST). If this solves the issues I was having with getting the |
| module.exports = function check( input, output, opts ) { | ||
| var processor = postcss([lost(opts)]); | ||
|
|
||
| expect(processor.process(input).css).to.equal(output); |
There was a problem hiding this comment.
I'm doing some smoke testing of this and I'm not sure I'm understanding the reasoning for this.
It causes a slight readability issue in the test output.
For example:
+ expected - actual
-a { *zoom: 1 }
+a { *zoom: 2 }
a:before { content: ''; display: table }
a:after { content: ''; display: table; clear: both }
I set the code to output *zoom: 2 so that should be the "actual" but instead it's reading as the "expected".
Typically when I'm working with tests I have the test represent the "expected".
There was a problem hiding this comment.
...okay, now I might be confusing myself. This change is correct. 🤔
http://chaijs.com/guide/styles/#expect
...for some reason reading through the failing tests has me pondering what "expected" and "actual" mean.
There was a problem hiding this comment.
the expected is what the result should be (the right result)
the actual is what your script is responding
expect(actual_result).toBe(expected_result);
There was a problem hiding this comment.
So looking at it again I still think there is something going on here that's not correct. I don't think it's the syntax that you changed...but I'm not sure what it is.
Here's a screencast that explains my thoughts.
http://quick.as/2LkHp0rQ
There was a problem hiding this comment.
@wbruno Any thoughts to this syntax is correct but the output is incorrect? ...am I mistaken in my understanding of what the output is suppose to be?
|
On another note: I've been doing a lot of soul searching about these tests. They're awesome but I also don't really know how practical they are for testing whether or not things are broken. They simply check whether or not the output matches what we want it to. (which isn't bad at all) Since most of the time the output is exactly the same as the input, the test written is sort of just a double check (which certainly isn't bad) instead of actually ensuring that LostGrid is performing properly. I want to look into getting some tests that actually will use LostGrid on a DOM and ensure the output is as expected visually not simply that the CSS matches. I'm looking into Sauce Labs to see if we might be able to get something going there that will check whether or not LostGrid is performing well visually. |
|
Yes! I will be great to have such tests, but for now, I was just focus in improve the coverage. |
|
@wbruno I don't disagree with improving coverage. 😄 |
|
@wbruno Is this still a WIP? |
|
It's ready. I will do more tests on another PR, and think about others ways to test. |
|
@wbruno Thanks, William! Do you have any additional thoughts on #315 (comment)? Before merging, I really want to understand what's happening with that. Other than that I'm super excited about improved coverage! 😄 Thank you! |
|
I saw the video and you are right, I'm sorry 😅 |
|
@wbruno I got really confused...the syntax there with what Chai is looking for isn't as straight-forward... If this is just adding tests I don't see any reason to not merge. Is this good to merge? 🍾 |
|
yep!! lets merge! |
|
@wbruno Want to update this with |
|
done |
2 participants
Adding some unit tests for better code coverage.
I also changed the
test/check.js, to show properly the expected and the result.