Skip to content

ENH: Increase coverage#2327

Merged
dzenanz merged 5 commits intoInsightSoftwareConsortium:masterfrom
jhlegarreta:IncreaseCoverage
Feb 24, 2021
Merged

ENH: Increase coverage#2327
dzenanz merged 5 commits intoInsightSoftwareConsortium:masterfrom
jhlegarreta:IncreaseCoverage

Conversation

@jhlegarreta
Copy link
Copy Markdown
Member

@jhlegarreta jhlegarreta commented Feb 18, 2021

Improvements concerning testing and a few related enhancements:

  • Fix an RTTI bug concerning a Superclass name.
  • Remove a duplicate include file.
  • Print all member variables in a number of classes.
  • Use itkBooleanMacro to set the boolean ivar values to On/Off.
  • Increase code coverage of a number of classes.

PR Checklist

  • No API changes were made (or the changes have been approved)
  • No major design changes were made (or the changes have been approved)
  • Added test (or behavior not changed)
  • Updated API documentation (or API not changed)
  • Added license to new files (if any)

@jhlegarreta jhlegarreta added type:Bug Inconsistencies or issues which will cause an incorrect result under some or all circumstances type:Enhancement Improvement of existing methods or implementation type:Testing Ensure that the purpose of a class is met/the results on a wide set of test cases are correct area:Filtering Issues affecting the Filtering module area:IO Issues affecting the IO module area:Registration Issues affecting the Registration module area:Segmentation Issues affecting the Segmentation module type:Coverage Code coverage impacts labels Feb 18, 2021
@jhlegarreta jhlegarreta mentioned this pull request Feb 18, 2021
Closed
5 tasks
@jhlegarreta
Copy link
Copy Markdown
Member Author

jhlegarreta commented Feb 18, 2021

Follow-up of the latest comments in PR #2286:

  • Commented the failing commands; will uncomment them in separate PRs once we are able to merge this.
  • Removed the baseline comparison for the classes for which I have some concerns on the appropriateness of the used data in the test.

Will push these outstanding things and some others as time permits, but will allow myself some time. I might suggest some of them as GitHub issues so as not to have them as a blocking element on a PR, and given that I might not find the time to push them forward.

dzenanz
dzenanz approved these changes Feb 18, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@dzenanz dzenanz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks good, pending fixing of the two failing tests.

@jhlegarreta
BUG: Fix Superclass name in RTTI macro
7d3e1fb 
Fix Superclass name in RTTI macro.
@jhlegarreta
COMP: Remove duplicate include file
c810d53 
Remove duplicate include file.
@jhlegarreta
ENH: Print all member variables.
d8a24aa 
Print all member variables.

Take advantage of the commit to
- Use the `itk::NumericTraits::PrintType` definition, the `print_helper`-enabled
  overload for array-like types, and the `itkPrintSelfObjectMacro` macro
  to improve how the ivars and objects that can be null pointers are printed.
- Print the member variables in the same order they were declared for the
  sake of consistency.
- Print the member variable names verbatim to conform to the ITK SW Guide.
@jhlegarreta
ENH: Use itkBooleanMacro for ImageSpacing, `InterpolateSurfaceLoc…
5d19a01 
…ation`

Use `itkBooleanMacro` to set `ImageSpacing` and
`InterpolateSurfaceLocation` boolean member variable values to On/Off:
- Deprecate `Set*(On/Off)` signatures.
- Test the methods.
@jhlegarreta
ENH: Increase code coverage
2032fa5 
Increase the code coverage:
- Add new unit tests for untested classes, including the corresponding
  baseline hash files.
- Exercise the basic object methods using the
  `ITK_EXERCISE_BASIC_OBJECT_METHODS` macro.
- Test the Set/Get methods using the `ITK_TEST_SET_GET_VALUE` macro.
- Test the boolean ivars using the `ITK_TEST_SET_GET_BOOLEAN` macro.
- Add the corresponding test arguments to the appropriate
  `CMakeLists.txt` test driver command.
- Remove the global `try/catch` blocks and use the
  `ITK_TRY_EXPECT_NO_EXCEPTION` macro around the statement that is
  liable to throw an exception to avoid boilerplate code and to improve
  readability.
- Test the exceptions.
- Remove explicit calls to the `Print` method and rely on the
  basic method exercising macro call.
- Improve slightly the test style to make them more consistent (e.g. input
  argument checking, test finishing message, make the types and variables
  dwell closer to the place where they are instantiated, define the image
  dimension as a constant; use aliases for the pixel types, etc.).
- Remove unnecessary print messages.
thewtex
thewtex approved these changes Feb 24, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@thewtex thewtex left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @jhlegarreta !

@dzenanz dzenanz merged commit 563b224 into InsightSoftwareConsortium:master Feb 24, 2021
@jhlegarreta jhlegarreta deleted the IncreaseCoverage branch February 24, 2021 20:55
@jhlegarreta
Copy link
Copy Markdown
Member Author

jhlegarreta commented Mar 7, 2021

A short note related to this PR:

Thanks for all the patience. I'm well aware that I put quite of a burden on the CI derived from some oversights in the commits. I'm sorry for the noise.

I had noticed that the coverage increased noticeably (89.86% to 89.96%) between Feb, 15 and Feb, 16. It did not look like a hiccup since it was stable for a few days days. Not sure if it was intended or was a nice side effect of the commits merged between those days, but that was good news.

And finally, this PR, standing on the shoulders of all previous efforts, made it possible to break the 90% barrier for the Aggregate Coverage build, and make the Code Coverage be over 88%, also breaking into the +1300 threshold for files with complete coverage. The aggregate coverage has been stable around 90.09% for a few days now.

So great effort from the maintainers and the community to push towards a tool that is better tested. I remember Luis' call to the mailing list in 2014 to increase the coverage :-). I think he would be glad to know this.

This was referenced May 15, 2021
Merged
Merged
Merged
@jhlegarreta jhlegarreta mentioned this pull request Oct 21, 2022
Merged
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@thewtex thewtex thewtex approved these changes

@dzenanz dzenanz dzenanz approved these changes

@hjmjohnson hjmjohnson Awaiting requested review from hjmjohnson

@blowekamp blowekamp Awaiting requested review from blowekamp

@N-Dekker N-Dekker Awaiting requested review from N-Dekker

@phcerdan phcerdan Awaiting requested review from phcerdan

Assignees

No one assigned

Labels

area:Filtering Issues affecting the Filtering module area:IO Issues affecting the IO module area:Registration Issues affecting the Registration module area:Segmentation Issues affecting the Segmentation module type:Bug Inconsistencies or issues which will cause an incorrect result under some or all circumstances type:Coverage Code coverage impacts type:Enhancement Improvement of existing methods or implementation type:Testing Ensure that the purpose of a class is met/the results on a wide set of test cases are correct

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@jhlegarreta @thewtex @hjmjohnson @dzenanz