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

Do not compile philox_test.cpp test code if unnamed lambdas are not used #1819

Closed
wants to merge 2 commits into from

Conversation

mmichel11
Copy link
Contributor

The recently added philox_test.cpp test does not do a preprocessor check if we are testing with unnamed lambda support, so we are seeing some compilation failures in CI.

I added this check. The rest of the Philox tests seem to be compiling fine.

Copy link
Contributor

@danhoeflinger danhoeflinger left a comment

Choose a reason for hiding this comment

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

I don't know enough about why we need to disable the RNG tests with unnamed lambdas off, but it seems this is the norm for the RNG tests.
Do we also need this for philox_unit_test.pass.cpp as well?
It seems that it calls return TestUtils::done(TEST_UNNAMED_LAMBDAS); but it does not actually skip the test with a preprocessor macro.
It seems like we need to add the guards, or change the done() call.

@aelizaro
Copy link
Contributor

aelizaro commented Sep 5, 2024

I don't know enough about why we need to disable the RNG tests with unnamed lambdas off, but it seems this is the norm for the RNG tests. Do we also need this for philox_unit_test.pass.cpp as well? It seems that it calls return TestUtils::done(TEST_UNNAMED_LAMBDAS); but it does not actually skip the test with a preprocessor macro. It seems like we need to add the guards, or change the done() call.

philox_unit_test doesn't contain any offload to devices, so it can be done with return TestUtils::done(TEST_UNNAMED_LAMBDAS); -> done() if it doesn't affect CI time significantly (I don't know in general when random numbers tests are run and when they are skipped)

@ElenaTyuleneva
Copy link
Contributor

Hi @mmichel11 , thanks for catching that. The issue was caused by my changes, I've fixed this thing and philox_unit_test.pass in a separate PR(#1820 ).

@mmichel11
Copy link
Contributor Author

@ElenaTyuleneva Thanks! Closing this PR.

@mmichel11 mmichel11 closed this Sep 5, 2024
@mmichel11 mmichel11 deleted the dev/mmichel11/philox_test_no_unnamed_lambda branch October 21, 2024 19:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test Test only Change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants