Skip to content

memtarget & memcapacity: tests and fixes - #1374

Merged
lukaszstolarczuk merged 3 commits into
oneapi-src:mainfrom
lukaszstolarczuk:tests-memtarget
Jun 17, 2025
Merged

memtarget & memcapacity: tests and fixes#1374
lukaszstolarczuk merged 3 commits into
oneapi-src:mainfrom
lukaszstolarczuk:tests-memtarget

Conversation

@lukaszstolarczuk

Copy link
Copy Markdown
Collaborator
  • add more tests (mostly negative cases)
  • change ifs to asserts in internal functions (i.a. to increase the coverage)
  • change return status in umfMemspaceMemtargetRemove, when trying to remove the last target - this is aligned with other functions, e.g. when trying to create memspace with size == 0 it throws the same error

@lukaszstolarczuk
lukaszstolarczuk requested a review from a team as a code owner June 12, 2025 10:25
@lukaszstolarczuk
lukaszstolarczuk force-pushed the tests-memtarget branch 3 times, most recently from 9f69e06 to 4c22546 Compare June 13, 2025 10:39
@bratpiorka
bratpiorka requested a review from lplewa June 13, 2025 10:46
@lplewa

lplewa commented Jun 16, 2025

Copy link
Copy Markdown
Contributor
  • mfMemspaceMemtargetRemove, when trying to remove the last target - this is aligned with other functions, e.g. when trying to create memspace with size == 0 it throws the same error

we have function

/// \brief Creates new empty memspace, which can be populated with umfMemspaceMemtargetAdd()
/// \param hMemspace [out] handle to the newly created memspace
/// \return UMF_RESULT_SUCCESS on success or appropriate error code on failure.
///
umf_result_t umfMemspaceNew(umf_memspace_handle_t *hMemspace);|

So this sentence is not true.

in internal memspace and memtarget functions.
previously we got OUT_OF_HOST_MEMORY while removing it.
Extend the testing to cover this.
@lukaszstolarczuk

Copy link
Copy Markdown
Collaborator Author
  • mfMemspaceMemtargetRemove, when trying to remove the last target - this is aligned with other functions, e.g. when trying to create memspace with size == 0 it throws the same error

we have function

/// \brief Creates new empty memspace, which can be populated with umfMemspaceMemtargetAdd()
/// \param hMemspace [out] handle to the newly created memspace
/// \return UMF_RESULT_SUCCESS on success or appropriate error code on failure.
///
umf_result_t umfMemspaceNew(umf_memspace_handle_t *hMemspace);|

So this sentence is not true.

Fair point. Changed.

@lplewa lplewa left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nit: Add test where you remove all targets from the memspace, and then add some.

@lukaszstolarczuk

Copy link
Copy Markdown
Collaborator Author

Nit: Add test where you remove all targets from the memspace, and then add some.

I'll add more tests in another iteration, thx

@lukaszstolarczuk
lukaszstolarczuk merged commit 3a4a450 into oneapi-src:main Jun 17, 2025
89 checks passed
@lukaszstolarczuk
lukaszstolarczuk deleted the tests-memtarget branch June 17, 2025 14:48
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.

3 participants