c3js / c3js/c3

Higher test coverage score needed to confidently accept pull requests

Open
#1,843 7 comments 1 reaction 1 assignee Claimed by @aendra-rininsland View on GitHub
R-needs-tests tests
Dominant language
JavaScript
Stars
9.3k
Forks
1.4k
Avg merge
6d 16h
Merged PRs (30d)
1

Description

This issue has changed in focus — previously I thought there were a lot of unit tests that didn't actually test anything, but in reality they were setting up other specs. This has been documented as #2047 and is fixed with #2050.

Thus the focus of this issue now is to improve test coverage.

### If you want to help, please pick a file with low coverage [(latest list here)](https://codecov.io/gh/c3js/c3/tree/d5afd6677a2da7b99651773dbb096df50dfc095f/src) and file a PR improving it!

## The following is outdated and left merely for historical purposes

I think part of why we've had so many regressive bugs added is because we're not actually unit testing huge swaths of the codebase.

I ran into the following line while troubleshooting an issue for a user:

`expect(true).toBeTruthy()`

This assertion _will always pass_. And it's _littered_ throughout the codebase.

https://github.com/c3js/c3/search?utf8=%E2%9C%93&q=%22expect%28true%29.toBeTruthy%28%29%22&type=Code

If you run `grep "expect(true).toBeTruthy()" spec/* | wc -l`, you get `140`. That means of the ~400 assertions made across all unit tests, 140 _mean absolutely nothing_.

I repeat:
#140 of our assertions test _absolutely nothing_

This is just one invalid assertion. For all we know, even more unit tests don't actually test what they're intended to test but look like they do.

Part of this is how C3 was originally constructed — because of a lack of modularity and how interconnected each piece is (particularly due to a reliance on concatenation), it wasn't really that easy to import a particular part of the library and run unit tests against it — you had to deal with the entire processed library and any additional side effects caused by that.

I'm not sure what the best answer is here. My thought is to just bin any unit tests with `expect(true).toBeTruthy` in them so as to reduce the coverage score, convert everything to Mocha as per #1838, and then add the appropriate tests, gradually increasing test coverage.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.