Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

BUG: Remove empty tests. #5129

Merged
merged 1 commit into from
Jan 10, 2025

Conversation

zivy
Copy link
Member

@zivy zivy commented Jan 9, 2025

Two of the GDCM tests were empty, did not test anything and were always successful (itkGDCMSeriesMissingDicomTagTest.cxx, itkGDCMSeriesReadImageWriteTest.cxx).

@github-actions github-actions bot added type:Bug Inconsistencies or issues which will cause an incorrect result under some or all circumstances type:Infrastructure Infrastructure/ecosystem related changes, such as CMake or buildbots type:Testing Ensure that the purpose of a class is met/the results on a wide set of test cases are correct area:IO Issues affecting the IO module labels Jan 9, 2025
@zivy zivy requested review from dzenanz and blowekamp January 9, 2025 19:28
@blowekamp
Copy link
Member

blowekamp commented Jan 9, 2025

I looked through the history... many changes to this empty file were made... but this is the change that marked the actual test as "legacy" then it was eventually removed.

e315539

Not sure if it did something useful. @seanm

@dzenanz dzenanz requested a review from seanm January 9, 2025 19:53
Copy link
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.

LGTM

Copy link
Contributor

@N-Dekker N-Dekker left a comment

Choose a reason for hiding this comment

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

Thanks @zivy Well spotted 👍

Just one remark: this is not really a bug fix, in my opinion. It's a clean-up action, which is a style improvement. (When an ITK user is interested to see the latest bug fixes that may affect their code, this is not one of them.)

So I would prefer a commit message subject line line:

STYLE: Remove empty GDCM tests

(I would also add the term "GDCM", to make the subject line a bit more specific.) See also the section "Commit Messages" in https://docs.itk.org/en/latest/contributing/index.html#commit-messages

Two of the GDCM tests were empty, did not test anything and were
always successful (itkGDCMSeriesMissingDicomTagTest.cxx,
itkGDCMSeriesReadImageWriteTest.cxx).
@zivy zivy force-pushed the removeEmptyTests branch from 9226ff0 to 5c40f31 Compare January 10, 2025 13:38
@zivy
Copy link
Member Author

zivy commented Jan 10, 2025

@N-Dekker Just changed the commit message subject line and force pushed. Surprisingly the PR subject line on GitHub remains the same after this change.

Copy link
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.

There is a difference between PR name, PR description, and commit message(s). PR name/description is taken from the first commit when it is opened, and does not get updated with additional (force) pushes.

@zivy zivy merged commit df23c1f into InsightSoftwareConsortium:master Jan 10, 2025
15 checks passed
@zivy zivy deleted the removeEmptyTests branch January 10, 2025 18:52
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
area:IO Issues affecting the IO module type:Bug Inconsistencies or issues which will cause an incorrect result under some or all circumstances type:Infrastructure Infrastructure/ecosystem related changes, such as CMake or buildbots 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
Development

Successfully merging this pull request may close these issues.

5 participants