Skip to content

Skip tests requiring NUMA if NUMA is not supported #655

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Aug 8, 2024
Merged

Skip tests requiring NUMA if NUMA is not supported #655

merged 1 commit into from
Aug 8, 2024

Conversation

EuphoricThinking
Copy link
Contributor

@EuphoricThinking EuphoricThinking commented Jul 31, 2024 •
edited
Loading

Description

Checks with numa_available() whether NUMA is supported on the system, then runs tests requiring NUMA according to the result.

  • Adds GTEST_SKIP() or substitutes GTEST_FAIL() with GTEST_SKIP() if failure has been determined by the lack of NUMA support;
  • removes UT_ASSERTs checking for NUMA if NUMA support has been checked in the test fixture;
  • allows uninstantiated tests in /test/provider_os_memory_multiple_numa_nodes.cpp, which suppresses failures caused by GoogleTestVerification test suite - the potentially uninstantiated tests are never run but skipped by the fixture if NUMA is not supported
  • a helper function get_available_numa_nodes() in /test/provider_os_memory_multiple_numa_nodes.cpp returns an empty vector in case of lacking NUMA support (which causes tests to be uninstantiated)
  • in examples/memspace, in case of NUMA not supported, the exit code is 0, however, the error message can be displayed with ctest --verbose instead of ctest --output-on-failure

Fixes #635

Yours first customer ❤️

Checklist

  • Code compiles without errors locally
  • All tests pass locally
  • CI workflows execute properly
  • CI workflows, not executed per PR (e.g. Nightly), execute properly
  • New tests added, especially if they will fail without my changes
  • Added/extended example(s) to cover this functionality
  • Extended the README/documentation
  • All newly added source files have a license
  • All newly added source files are referenced in CMake files
  • Logger (with debug/info/... messages) is used

@EuphoricThinking EuphoricThinking requested a review from a team as a code owner July 31, 2024 11:35
lplewa
lplewa requested changes Jul 31, 2024
View reviewed changes
examples/memspace/memspace_hmat.c Outdated
@@ -63,7 +63,7 @@ int main(void) {
// Check if NUMA is available
if (numa_available() < 0) {
fprintf(stderr, "NUMA is not available on this system.\n");
return -1;
return 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

please return test_skip_error_code (see if below)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done; added a define with test_skip_error_code in examples/common/utils_examples.h

PatKamin
PatKamin reviewed Aug 1, 2024
View reviewed changes
@EuphoricThinking EuphoricThinking force-pushed the skipping1 branch 9 times, most recently from 555e5ee to 857324b Compare August 2, 2024 13:47
lplewa
lplewa requested changes Aug 2, 2024
View reviewed changes
.gitignore Outdated
@@ -63,6 +63,7 @@ docs/
# Build files
/build*/
out/
/test/build/
Copy link
Contributor

Choose a reason for hiding this comment

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

??

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have removed that part

.gitignore Outdated
@@ -79,6 +80,7 @@ out/

# Temporary files
*.~vsdx
..gitignore.swp
Copy link
Contributor

Choose a reason for hiding this comment

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

??

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am going to create a new PR with this line changed to *.swp

examples/CMakeLists.txt Outdated
@@ -167,6 +167,8 @@ else()
endif()

if(LINUX)
set(SKIP_CODE 125)
Copy link
Contributor

Choose a reason for hiding this comment

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

UMF_TEST_SKIP_RETURN_CODE

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

examples/common/utils_examples.h Outdated

#define test_skip_error_code 125

#endif /* UTILS_EXAMPLES_H */
Copy link
Contributor

Choose a reason for hiding this comment

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

add new line at the end of the file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

test/common/numa_helpers.h Outdated
@@ -12,6 +12,9 @@

#include "test_helpers.h"

// Needed for CI
#define test_skip_error_code 125
Copy link
Contributor

Choose a reason for hiding this comment

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

Better to define it in test_helpers.h

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, however, I am not sure where I have chosen the right location in test_helpers.h

@EuphoricThinking EuphoricThinking force-pushed the skipping1 branch 4 times, most recently from fbb4297 to 92882b0 Compare August 6, 2024 20:18
ldorau

This comment was marked as resolved.

Sign in to view

@ldorau ldorau changed the title Skipping tests requiring NUMA if NUMA is not supported Skip tests requiring NUMA if NUMA is not supported Aug 7, 2024
ldorau
ldorau requested changes Aug 7, 2024
View reviewed changes
lukaszstolarczuk
lukaszstolarczuk reviewed Aug 7, 2024
View reviewed changes
Copy link
Contributor

@lukaszstolarczuk lukaszstolarczuk left a comment

Choose a reason for hiding this comment

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

FYI, your commit is veeery descriptive - it's not required to describe, line be line, what you did in the change (we can see that in the commit itself), it's rather useful to describe it high level and provide a reason why. In here it's ok, because you're fixing an issue, so all the details should be there :)

@EuphoricThinking
fix: skip tests requiring NUMA if NUMA is not supported
80e6459 
Substitute GTEST_FAIL() with GTEST_SKIP() or add GTEST_SKIP().
Remove UT_ASSERTs checking for NUMA.
Enable skipping tests in CMakeLists.txt.
Add UMF_TEST_SKIP_RETURN_CODE (=125) to CMakeLists.txt.
Add test_skip_error_code (=125) to helper header files
for examples and tests.
Allow uninstantiated tests in
/test/provider_os_memory_multiple_numa_nodes.cpp.
Make get_available_numa_nodes() return an empty vector
in the environment without NUMA.

Fixes: oneapi-src#635

Signed-off-by: Agata Momot <[email protected]>
@lukaszstolarczuk lukaszstolarczuk merged commit 7ba1625 into oneapi-src:main Aug 8, 2024
65 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ldorau ldorau ldorau approved these changes

@lplewa lplewa lplewa approved these changes

@lukaszstolarczuk lukaszstolarczuk lukaszstolarczuk approved these changes

@PatKamin PatKamin PatKamin approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Cannot run ctest on WSL2 due to the lack of NUMA
5 participants
@EuphoricThinking @ldorau @lplewa @lukaszstolarczuk @PatKamin