Improve test coverage

[eps1lon]

@eps1lon We have lost some test coverage:

• https://codecov.io/gh/mui-org/material-ui/src/6e430498eff65818d11346368b2b6b2e519f68b4/packages/material-ui/src/ButtonBase/ButtonBase.js#L216
• https://codecov.io/gh/mui-org/material-ui/src/6e430498eff65818d11346368b2b6b2e519f68b4/packages/material-ui/src/BottomNavigationAction/BottomNavigationAction.js#L68
• https://codecov.io/gh/mui-org/material-ui/src/6e430498eff65818d11346368b2b6b2e519f68b4/packages/material-ui/src/ExpansionPanelSummary/ExpansionPanelSummary.js#L86

I'm not sure yet what's going on. Codecov reports 100% test coverage, and yet, it's not the case after the merge. Can we trust codecov 🤔?

Originally posted by @oliviertassinari in #14536 (comment)

[eps1lon]

Not going to pursue this. 100% test coverage is (and I don't want to sugarcoat this) useless. Just the fact that we measure line coverage removes any value from this number. For reporting yes, but as a limit on a passing build, no.

If anybody wants to pick this up: just add istanbul ignore if comments. It's what we're doing in all the other places to display a nice number.

[oliviertassinari]

@eps1lon They are different topics here, by order of importance:

• Codcov didn't report the test coverage change. I think that it's the most important issue. We should know about any test coverage changes!
• I think that we should try not to decrease test coverage, I would consider it's a good practice to keep it constant or improve it. It should fail the build when reducing test coverage (something we can still override, it's a tradeoff). In the past, we were able to remove the dead branches code after looking at the test coverage missing lines, we have noticed that it was impossible to reach some conditions.
• We have 13 occurrences of istanbul ignore if in the codebase. 5 are temporary and going away with [styles] Remove the old styles modules #14560, 4 are here to reduce bundle size on the dead prop-types code. 3 are untested warnings (I think that we should work on them). 1 is for hiding the fact that we don't have any true server-side rendering test. So, we can easily remove 8 occurrences. Now, to remove the 5 others occurrences we need to add two new test configurations: production & server-side environment. So far the tradeoff was to ignore the production and server-side case. We can revisit it.
• Is 100% test coverage useless? It will definitely harm if your product is nontechnical. In our case, 100% test coverage was only made possible thanks to @agamrafaeli, thank you! We shouldn't have 100% as a "goal", we should keep test coverage as a tool to improve the components. Improving test coverage has often been correlated with implementation improvements, so I'm happy anytime somebody increases the coverage. For instance, [styles] 100% test coverage #14566 has helped me fix some broken part of the implementation, I don't regret this time investment.

---

At the end of the day, our true test coverage is our developer's users, test coverage is only here to reduce the feedback loop: to save us time. If 100% test coverage slows us down, then it's a bad idea!

[oliviertassinari]

We value test coverage, if somebody wants to increase it, we would love to accept pull requests for it! Until then I'm closing.