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.

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

Increase test coverage #40

Closed
kettanaito opened this issue Jul 3, 2018 · 7 comments
Closed

Increase test coverage #40

kettanaito opened this issue Jul 3, 2018 · 7 comments
Labels
needs:tests This issue needs tests. Desperately.
Milestone

Comments

@kettanaito
Copy link
Owner

kettanaito commented Jul 3, 2018

What

Need to increase code coverage up to 100%.

Why

To have our code covered with tests, of course. And also because 99% coverage is simply unacceptable.

How

@kettanaito kettanaito added the needs:tests This issue needs tests. Desperately. label Jul 3, 2018
@kettanaito kettanaito changed the title Increase code coverage Increase test coverage Jul 3, 2018
@kettanaito kettanaito added this to the Optimization milestone Jul 18, 2018
@kettanaito
Copy link
Owner Author

Added unit tests for invariant function: a6bb237

@Vidlec
Copy link
Contributor

Vidlec commented Jul 19, 2018

I was writing tests to cover missing branches and found out that in
src/utils/templates/getAreasList.js

      const propValue = sanitizeTemplateString(props[propName])
      const nextAreas = propValue ? res.areas.concat(propValue) : res.areas

propValue can be never undefined or false because sanitizeTemplateString always returns an array.

So either check for array length or remove this check.
What do you think?

@kettanaito
Copy link
Owner Author

@Vidlec You are completely right!

[].concat(2) // [2]
// is indeed the same as
[].concat([2]) // [2]

Yes, we should remove that check and just concat the received sanitized template string to res.areas.

If you can, you can add that via pull request, that would be massively appreciated! If not, just create a ticket so we don't forget about that.


Also, ❤️ for increasing test coverage.

@Vidlec Vidlec mentioned this issue Jul 20, 2018
3 tasks
@Vidlec
Copy link
Contributor

Vidlec commented Jul 20, 2018

There is a one last thing to have 100% coverage

if (prefix === 'min') {
      if (includesArea) {
        if (behavesSame || behavesInclusive) {
          nextValue = breakpointB[propName]
        }
      } else {
        if (shouldStretch) {
          nextValue = breakpointB[propName]
        }
      }
    }

That else branch is uncovered. I dont dare writing test for this, because I have no idea how this works 😃

@kettanaito
Copy link
Owner Author

@Vidlec 😄

I think this is mergeBreakpoints function, isn't it?
That fires when you merge a minABC property of the breakpoint and the template declaration doesn't include the current area, and this area must be stretched. I doubt that makes the picture any cleaner for you...

I can explain you in person, just ping me anytime. It's nothing complicated, I promise.

@kettanaito
Copy link
Owner Author

Test coverage has increased to 100% by merging your pull request #47.

Much thanks and welcome to contributors!

@kettanaito
Copy link
Owner Author

@Vidlec Please let me know if we should reopen this issue to have that last branch covered. I have overlooked the comment and have already closed it. Sorry.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs:tests This issue needs tests. Desperately.
Projects
None yet
Development

No branches or pull requests

2 participants