Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

BUG: fixed test that made bad assumptions of GetGlobalDefaultNumberOf… #5191

Merged
merged 1 commit into from
Jan 30, 2025

Conversation

seanm
Copy link
Contributor

@seanm seanm commented Jan 27, 2025

…Threads()

@seanm seanm marked this pull request as draft January 27, 2025 17:24
@github-actions github-actions bot added type:Bug Inconsistencies or issues which will cause an incorrect result under some or all circumstances type:Testing Ensure that the purpose of a class is met/the results on a wide set of test cases are correct area:Core Issues affecting the Core module labels Jan 27, 2025
@seanm
Copy link
Contributor Author

seanm commented Jan 27, 2025

@dzenanz how would I solve the case with the ITK_TEST_SET_GET_VALUE macro? I'm not sure what to use instead...

@seanm seanm force-pushed the max-thread-test-bug branch from 74576b6 to 0bb2672 Compare January 27, 2025 19:22
@seanm seanm marked this pull request as ready for review January 27, 2025 19:52
@seanm seanm force-pushed the max-thread-test-bug branch from 0bb2672 to a603670 Compare January 28, 2025 00:09
@N-Dekker
Copy link
Contributor

Thanks @seanm !

@seanm
Copy link
Contributor Author

seanm commented Jan 28, 2025

There's one more failing test that I hope to figure out and add here...

@seanm
Copy link
Contributor Author

seanm commented Jan 28, 2025

OK, I don't get it... need help :)

It's this code:

ITK_TRY_EXPECT_EXCEPTION(dummySrc->Update());

The test fails with Failed to catch expected exception.

The call to dummySrc->SetNumberOfWorkUnits(4) will be clamped down from 4 to 2 (my ITK_DEFAULT_MAX_THREADS), so I thought changing the hardedcoded for loop from 4 would fix it, but alas no (though that change maybe necessary too, but insufficient).

@dzenanz
Copy link
Member

dzenanz commented Jan 28, 2025

Nothing jumps at me. I guess some digging is needed into DummyImageSource.

GetGlobalDefaultNumberOfThreads() and GetNumberOfWorkUnits() in fact can return less that you ask it to, because it's also clamped to the compile-time choice of ITK_DEFAULT_MAX_THREADS.
@seanm seanm force-pushed the max-thread-test-bug branch from a603670 to d200623 Compare January 28, 2025 19:54
@seanm
Copy link
Contributor Author

seanm commented Jan 28, 2025

I took a wild guess: there was another 4 in the code. Changing that fixed it.

@seanm
Copy link
Contributor Author

seanm commented Jan 30, 2025

If this looks good to y'all, would be nice to merge, as it will green up cdash a bit.

@dzenanz dzenanz merged commit 7414f89 into InsightSoftwareConsortium:master Jan 30, 2025
15 checks passed
@seanm
Copy link
Contributor Author

seanm commented Jan 30, 2025

Seems this broke the build, depending on build options: https://open.cdash.org/viewBuildError.php?buildid=10179364

fix: #5200

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:Core Issues affecting the Core module type:Bug Inconsistencies or issues which will cause an incorrect result under some or all circumstances 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.

3 participants