[propasal] Add unit tests #133

Closed
tymondesigns opened this Issue Aug 8, 2016 · 18 comments

Projects

None yet

7 participants

@tymondesigns
Contributor

So, on a scale of "Hell No" to "Hell yeah" how interested is everybody in adding some unit tests?

I would propose adding this package to handle the testing of the functions and mixins.

for example:

@include test('Font Size [mixin]') {
  @include assert('Outputs a font size and line height based on a given value.') {
    @include output {
      @include font-size(16px);
    }

    @include expect {
      font-size: 16px;
      font-size: 1rem;
      line-height: 1.5;
    }
  }
}

Again I'd be happy to get the ball rolling on this...

@anselmh
Member
anselmh commented Aug 8, 2016

Hey, thanks for the suggestion. I’m a bit split on this topic. Usually I think testing code is a desirable goal but testing CSS doesn’t work very well and needs crazy stuff to make it work (the code to write tests, the lib that you suggested) and it requires a pretty high level of knowledge on this unit testing if you want to contribute code.

Therefore I’m currently voting not to do it here, but this is something we should discuss / agree on as a whole team.
So, @inuitcss/core please vote 👎 or 👍 here on my post. Thanks!

@nenadjelovac
Member
nenadjelovac commented Aug 8, 2016 edited

@tymondesigns thanks for the suggestion! However, I have to vote 👎 because of the reasons @anselmh stated, and because of the requirement that simplicity is paramount. Hope it makes sense.

@csswizardry
Member

Holy s—t, this is insane: I was literally just thinking about adding tests to inuitcss! I wasn’t sure how best to do it (external lib or writing something small to cover our specific mixins) but let me take a look at true.

@csswizardry
Member

@nenadjelovac: I don’t think this encroaches on simplicity: tests will allow us to refactor any mixins etc. and be immediately notified about breakages. They would likely be private to the framework.

@tymondesigns
Contributor

@csswizardry My thoughts exactly. The external end-user experience will be unaffected, and this will simply serve as an internal tool to provide the confidence when refactoring code.

@nenadjelovac
Member

@csswizardry

They would likely be private to the framework.

if that is the case i'd probably change my vote, but I'd have to see how it's gonna get implemented ;)

@tymondesigns
Contributor

If no-one beats me to it, I can knock a little POC together this evening when I'm home...

@anselmh
Member
anselmh commented Aug 8, 2016

So we’d only test internal functions and mixins, right? This would be fine for me. Still not sure if it’s a good idea but if you all agree, then let’s do this.

@nicoqh
nicoqh commented Aug 8, 2016 edited

Tests don't add complexity to userland code. It can even be a dev dependency, and gives a lot more confidence :)

@csshugs
Member
csshugs commented Aug 8, 2016

Just want to throw in the question: Is inuitcss' Sass stuff really that complex to justify adding the complexity of unit tests? Becuase although it's not complexity for inuitcss' users, it is complexity for us (read: the inuitcss dev team).

I mean, this is not a huge Sass library a la Bourbon with much complex stuff in it. The most complex Sass code in inuitcss at the moment is probably this.

I'm just sharing my opinion here which is: I wouldn't need Sass unit tests in inuitcss.

If you guys consider it being a good idea though, I'm on board.

@csshugs csshugs added the enhancement label Aug 8, 2016
@tymondesigns tymondesigns added a commit to tymondesigns/inuitcss that referenced this issue Aug 8, 2016
@tymondesigns tymondesigns [refs #133] adding initial setup and tests 7b0d963
@tymondesigns
Contributor

I have created the PR #134 so that we can see what it might look like.

here's a screenshot of the terminal for reference.

image

@csshugs I can understand your point of view, and I somewhat agree, but for me at least I don't see it as being that complicated to write the tests, versus the benefits they would yield.

@csswizardry Do you have any more thoughts on this now that there's some code to look at?

@tymondesigns tymondesigns added a commit to tymondesigns/inuitcss that referenced this issue Aug 8, 2016
@tymondesigns tymondesigns [refs #133] strict mode e68a15a
@florianbouvot
Member

Like @csshugs and @anselmh I'm not sure it's necessary for this kind of project.

@anselmh
Member
anselmh commented Aug 8, 2016

So, my initial feedback was based on the idea of assuming that all code of inuitcss will be unit tested. This is something I’d strongly object, since it makes contributions too hard and would also require a lot of additional work by all core contributors not only initially but also over-time.

However, testing our internal mixins and functions only, makes totally sense to me in a sense that we have easy unit test results and a failure indicator when we refactor something or add a new feature. I don’t think it’d hurt to add these few tests here, to be honest.

@tymondesigns tymondesigns added a commit to tymondesigns/inuitcss that referenced this issue Aug 8, 2016
@tymondesigns tymondesigns [refs #133] adding initial setup and tests
[refs #133] strict mode
6a25c29
@tymondesigns tymondesigns added a commit to tymondesigns/inuitcss that referenced this issue Aug 8, 2016
@tymondesigns tymondesigns [refs #133] match indentation 9c9fb04
@tymondesigns tymondesigns added a commit to tymondesigns/inuitcss that referenced this issue Aug 8, 2016
@tymondesigns tymondesigns [refs #133] adding initial setup and tests
[refs #133] strict mode

[refs #133] match indentation

[refs #133] consistent quotes
945b620
@tymondesigns tymondesigns added a commit to tymondesigns/inuitcss that referenced this issue Aug 8, 2016
@tymondesigns tymondesigns [refs #133] add test to npmignore 6603a6d
@csswizardry
Member

Tests (in general) are a great idea IMO. It gives us programmatic safety. I’m stuck in a coffee shop in the middle of Buenos Aires with 10% battery on my laptop right now, so I can’t take a close look, but thanks so much, @tymondesigns! I’ll delve into this ASAP :)

@tymondesigns
Contributor

@csswizardry no problem! 😄

@tymondesigns tymondesigns added a commit to tymondesigns/inuitcss that referenced this issue Aug 8, 2016
@tymondesigns tymondesigns [refs #133] test oldIE option 038a7c5
@nenadjelovac
Member

I thought the same as @anselmh! But now I get it.

While on the topic, shouldn't we lint the code as well?

@anselmh
Member
anselmh commented Aug 8, 2016

@nenadjelovac different, topic so please create a new issue for it. In short: Yes, we should, if possible – preferrably by using http://stylelint.io/ :)

@tymondesigns tymondesigns added a commit to tymondesigns/inuitcss that referenced this issue Aug 8, 2016
@tymondesigns tymondesigns [refs #133] tweaks bf0b6ad
@tymondesigns tymondesigns added a commit to tymondesigns/inuitcss that referenced this issue Aug 8, 2016
@tymondesigns tymondesigns [refs #133] adding widths test (doesn't work currently and tweaking t…
…est desc)
20f46c2
@tymondesigns tymondesigns added a commit to tymondesigns/inuitcss that referenced this issue Aug 8, 2016
@tymondesigns tymondesigns [refs #133] typo 9175a7c
@tymondesigns tymondesigns added a commit to tymondesigns/inuitcss that referenced this issue Aug 9, 2016
@tymondesigns tymondesigns [refs #133] glob tests 03d3ce7
@tymondesigns tymondesigns added a commit to tymondesigns/inuitcss that referenced this issue Aug 9, 2016
@tymondesigns tymondesigns [refs #133] adding initial setup and tests
[refs #133] strict mode

[refs #133] match indentation

[refs #133] consistent quotes

[refs #133] add test to npmignore

[refs #133] test oldIE option

[refs #133] tweaks

[refs #133] adding widths test (doesn't work currently and tweaking test desc)

[refs #133] typo

[refs #133] glob tests

[refs #133] indentation
d8961cc
@tymondesigns tymondesigns added a commit to tymondesigns/inuitcss that referenced this issue Aug 10, 2016
@tymondesigns tymondesigns [refs #133] adding initial reference to unit tests 00190b8
@tymondesigns tymondesigns added a commit to tymondesigns/inuitcss that referenced this issue Aug 10, 2016
@tymondesigns tymondesigns [refs #133] further amends 8772a0d
@anselmh
Member
anselmh commented Aug 10, 2016

This landed in 6ce1a81.

@anselmh anselmh closed this Aug 10, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment