vulkan: Remove redundant negative testing #2078
Conversation
joshqti
commented
Sep 10, 2024
joshqti
commented
- negative testing for semaphore functions is accomplished in semaphore tests, as well as create image in the api test
| @@ -323,27 +323,6 @@ int test_consistency_external_image(cl_device_id deviceID, cl_context _context, | |||
| test_error(errNum, "Unable to create Image with Properties"); | |||
| image.reset(); | |||
|
|
|||
| // Passing image_format as NULL | |||
| image = clCreateImageWithProperties(context, extMemProperties.data(), | |||
There was a problem hiding this comment.
This is not related to semaphore negative tests, but for image import negative testing and should be retained, right?
There was a problem hiding this comment.
These failure conditions are already described in the OpenCL base specification. If coverage is missing, they should be added to one of the core tests, such as API or basic. The exception is if cl_khr_external_memory has a special provision for image_format or image_desc that I am forgetting.
| @@ -158,47 +158,6 @@ int test_consistency_external_for_1dimage(cl_device_id deviceID, | |||
| test_error(errNum, "Unable to create Image with Properties"); | |||
| image.reset(); | |||
|
|
|||
| // Passing properties, image_desc and image_format all as NULL | |||
| image = clCreateImageWithProperties(context, NULL, CL_MEM_READ_WRITE, NULL, | |||
There was a problem hiding this comment.
I think this should be retained for the same reason I mentioned above.
There was a problem hiding this comment.
This test case does not make sense for this extension, there is no information being passed that assosciates clCreateImageWithProperties and cl_khr_external_memory.
If we believe this is important coverage, it would be a better fit in api or basic.
There was a problem hiding this comment.
I prefer we retain the coverage here until we add it to more appropriate place rather than removing it right away.
We can either add the same to more appropriate tests or move these tests to more appropriate place in a subsequent PR.
There was a problem hiding this comment.
Sure, we can leave in the two valid negative tests. I made an issue to cover these #2139
| @@ -162,47 +162,6 @@ int test_consistency_external_for_3dimage(cl_device_id deviceID, | |||
| test_error(errNum, "Unable to create Image with Properties"); | |||
| image.reset(); | |||
|
|
|||
| // Passing properties, image_desc and image_format all as NULL | |||
| image = clCreateImageWithProperties(context, NULL, CL_MEM_READ_WRITE, NULL, | |||
There was a problem hiding this comment.
This test case is ambiguous. both format and image desc are null, either case is an error, and the OpenCL spec does specify if CL_INVALID_IMAGE_FORMAT_DESCRIPTOR or CL_INVALID_IMAGE_DESCRIPTOR is to be returned if both are NULL.
This test case does not make sense for this extension, there is no information being passed that assosciates clCreateImageWithProperties and cl_khr_external_memory.
If we believe this is important coverage, it would be a better fit in api or basic.
|
Discussed in the October 15th memory subgroup. Waiting for @nikhiljnv to respond. |
|
@joshqti |
- clCreateImageWithProperties does not specify which error takes precedence when two parameters that must not be null are Change-Id: I9b057c6778033b031281a6ba666785d3a5f340d3
joshqti
force-pushed
the
delete_redundant_tests
branch
from
73e757a
to
c78eff8
Compare
|
Merging as discussed in memory subgroup call on November 5th 2024. |
