trilinos / Trilinos

Primary repository for the Trilinos Project
https://trilinos.org/
Other
1.22k stars 570 forks source link

Trilinos needs automated coverage testing #3958

Closed bartlettroscoe closed 3 years ago

bartlettroscoe commented 6 years ago

@trilinos/framework

Trilinos currently has no coverage testing. The last coverage build posted to CDash was for MueLu back on 8/20/2018 as shown in this query.

We need regular automated coverage builds posting to CDash.

csiefer2 commented 6 years ago

I've generally found coverage testing to demonstrate a few issues, but be buried in an avalanche of false alarms based on debugging code and/or error messages which never execute in the tests.

What is the value proposition here?

bartlettroscoe commented 6 years ago

I've generally found coverage testing to demonstrate a few issues, but be buried in an avalanche of false alarms based on debugging code and/or error messages which never execute in the tests.

You can write unit tests to trigger all of that. With proper test-driven development a few helper macros (to disable defensive code that should never get triggered or unreachable returns, etc.) it is pretty easy to get near 100% line coverage. Any code not under automated testing has 0% confidence.

What is the value proposition here?

We can't currently tell if test coverage is getting better or worse without automated coverage builds.

It is insane a project the size and importance of Trilinos has no coverage testing and does not take coverage seriously.

bartlettroscoe commented 6 years ago

@csiefer2, and according to this policy:

Production Trilinos code should have almost 100% line and feature coverage.

csiefer2 commented 6 years ago

You can write unit tests to trigger all of that. With proper test-driven development a few helper macros (to disable defensive code that should never get triggered or unreachable returns, etc.) it is pretty easy to get near 100% line coverage. Any code not under automated testing has 0% confidence.

Sure, you can. But nobody will.

bartlettroscoe commented 6 years ago

Sure, you can. But nobody will.

I do. Otherwise, you can't trust your error handling.

csiefer2 commented 6 years ago

It is insane a project the size and importance of Trilinos has no coverage testing and does not take coverage seriously.

Insane, perhaps. But Trilinos is not unique in this regard. We used to do coverage testing for (insert engineering code here). We stopped because the signal-to-noise ratio was way too low for it to be remotely useful.

bartlettroscoe commented 6 years ago

We stopped because the signal-to-noise ratio was way too low for it to be remotely useful.

@csiefer2, lots of examples of bad SE practices. Trilinos should be better. Trilinos customers need Trilinos to be better.

csiefer2 commented 6 years ago

I do. Otherwise, you can't trust your error handling.

I salute your dedication to testing, sir.

csiefer2 commented 6 years ago

@csiefer2, lots of examples of bad SE practices. Trilinos should be better. Trilinos customers need Trilinos to be better.

Don't get me wrong, there's nothing wrong with running together code coverage tests, so long as it doesn't stop @trilinos/framework from doing something more important. I'm just skeptical that they'll make any meaningful impact on developer behavior.

bartlettroscoe commented 6 years ago

I salute your dedication to testing, sir.

@csiefer2, if one can make it all the way through the book Working Effectively with Legacy Code without adopting a "dedication to testing" then one should not be writing production quality software.

bartlettroscoe commented 6 years ago

I'm just skeptical that they'll make any meaningful impact on developer behavior.

It will if it is part of PR testing. For example, coverage has to go up or be the same in order to accept a PR. Forcing compliance is the only way to viable way to "impact developer behavior".

csiefer2 commented 6 years ago

@csiefer2, if one can make it all the way through the book Working Effectively with Legacy Code without adopting a "dedication to testing" then one should not be writing production quality software.

So you recommend that book then?

csiefer2 commented 6 years ago

It will if it is part of PR testing. For example, coverage has to go up or be the same in order to accept a PR. Forcing compliance is the only way to viable way to "impact developer behavior".

This is where you and I strongly differ in opinion :)

bartlettroscoe commented 6 years ago

@csiefer2, if one can make it all the way through the book Working Effectively with Legacy Code without adopting a "dedication to testing" then one should not be writing production quality software.

So you recommend that book then?

@csiefer2, one of the most influential (and humbling) books I have ever read.

csiefer2 commented 6 years ago

@csiefer2, one of the most influential (and humbling) books I have ever read.

Cool! I'll have to stick it on the reading list then.

bartlettroscoe commented 6 years ago

It will if it is part of PR testing. For example, coverage has to go up or be the same in order to accept a PR. Forcing compliance is the only way to viable way to "impact developer behavior".

This is where you and I strongly differ in opinion :)

It is pretty obvious which direction things are going. And Trilinos is behind in this trend if you look at other projects.

csiefer2 commented 6 years ago

It is pretty obvious which direction things are going. And Trilinos is behind in this trend if you look at other projects.

It depends on your basis for comparison, I suppose. Trilinos looks way ahead of the curve from this chair (to the point where productivity is negatively impacted).

bartlettroscoe commented 5 years ago

Would be great if we had one build with coverage results. Would help in the code review process.

github-actions[bot] commented 3 years ago

This issue has had no activity for 365 days and is marked for closure. It will be closed after an additional 30 days of inactivity. If you would like to keep this issue open please add a comment and/or remove the MARKED_FOR_CLOSURE label. If this issue should be kept open even with no activity beyond the time limits you can add the label DO_NOT_AUTOCLOSE. If it is ok for this issue to be closed, feel free to go ahead and close it. Please do not add any comments or change any labels or otherwise touch this issue unless your intention is to reset the inactivity counter for an additional year.

github-actions[bot] commented 3 years ago

This issue was closed due to inactivity for 395 days.