Skip to content

Conversation

@lukaszstolarczuk
Copy link
Contributor

  • 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
Copy link
Contributor

lplewa commented Jun 16, 2025

  • 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
Contributor 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.

Copy link
Contributor

@lplewa lplewa left a comment

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
Contributor 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