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

Add tests to ScopeGuard #7028

Merged
merged 15 commits into from
May 30, 2024
Merged

Add tests to ScopeGuard #7028

merged 15 commits into from
May 30, 2024

Conversation

pzehner
Copy link
Contributor

@pzehner pzehner commented May 23, 2024

This PR aims to test the Kokkos::ScopeGuard class, that was untested.

No new features nor modifications were added to the core source code. A new test file core/unit_test/UnitTest_ScopeGuard.cpp was created and tests the scope guard on various configurations. Especially, faulty situations were tested, e.g. when creating two scope guards.

@pzehner
Copy link
Contributor Author

pzehner commented May 24, 2024

Tests seem to fail for NVHPC 23.7 + CUDA 12.2. I could successfully execute them on NVHPC 24.5 + CUDA 12.2, however. Il will reproduce using the same Docker image as for the tests. I also had segfaults on exit tests and on death tests using a local install of NVHPC 23.11. Maybe the tests should not run for this compiler?

@pzehner pzehner marked this pull request as ready for review May 24, 2024 15:11
Copy link
Member

@dalg24 dalg24 left a comment

Choose a reason for hiding this comment

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

You want to static assert that the scope guard is not copyable?

static_assert(!std::is_copy_constructible<Kokkos::ScopeGuard>
              && !std::is_copy_assignable<Kokkos::ScopeGuard>);

(Add at namespace scope)

I think our convention is to suffix death tests with _DeathTest.
I am not sure how to do it with fixtures though.

core/unit_test/UnitTest_ScopeGuard.cpp Outdated Show resolved Hide resolved
core/unit_test/UnitTest_ScopeGuard.cpp Outdated Show resolved Hide resolved
@dalg24
Copy link
Member

dalg24 commented May 24, 2024

Tests seem to fail for NVHPC 23.7 + CUDA 12.2. I could successfully execute them on NVHPC 24.5 + CUDA 12.2, however. Il will reproduce using the same Docker image as for the tests. I also had segfaults on exit tests and on death tests using a local install of NVHPC 23.11. Maybe the tests should not run for this compiler?

Look a bit what we did for other death tests. I think we generally remove them from the list with the OpenACC backend.

@pzehner
Copy link
Contributor Author

pzehner commented May 27, 2024

You want to static assert that the scope guard is not copyable?

This is a good idea, I will do it.

I think our convention is to suffix death tests with _DeathTest.
I am not sure how to do it with fixtures though.

Do you mean to suffix the test suite name or the test name? The former is passed as a fixture class, but renamed to be meaningful in the outputs; so I can append _DeathTest to by renaming the same fixture class differently.

@pzehner
Copy link
Contributor Author

pzehner commented May 27, 2024

Look a bit what we did for other death tests. I think we generally remove them from the list with the OpenACC backend.

But the OpenACC backend works fine on those death tests! It is the CUDA one with NVHPC which is faulty. This is weird, I should check what was done for the other death tests.

dalg24
dalg24 previously approved these changes May 28, 2024
core/unit_test/UnitTest_ScopeGuard.cpp Show resolved Hide resolved
@dalg24
Copy link
Member

dalg24 commented May 28, 2024

NVC++ does not seem to like these new tests being added. The OPENMPTARGET and the NVHPC build failures are legit.
You might need to disable them. See #7028 (comment)

@dalg24 dalg24 dismissed their stale review May 28, 2024 14:44

Need to address CI failures with NVC++

nliber
nliber previously requested changes May 28, 2024
core/unit_test/UnitTest_ScopeGuard.cpp Show resolved Hide resolved
@pzehner
Copy link
Contributor Author

pzehner commented May 29, 2024

Ok, I'll add some guards to prevent those tests to run with NVHPC.

Yet, I'm surprised that NVHPC with the OpenACC backend works.

@pzehner
Copy link
Contributor Author

pzehner commented May 30, 2024

It seems to work now for NVHPC. The only failing job is for HIP, which segfaults for a lot of tests.

@masterleinad masterleinad requested a review from nliber May 30, 2024 14:39
@dalg24 dalg24 dismissed nliber’s stale review May 30, 2024 15:18

assertions were added

@dalg24 dalg24 merged commit f0704b3 into kokkos:develop May 30, 2024
28 of 29 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants