-
Notifications
You must be signed in to change notification settings - Fork 199
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
vulkan: Remove redundant negative testing #2078
base: main
Are you sure you want to change the base?
Conversation
joshqti
commented
Sep 10, 2024
- negative testing for semaphore functions is accomplished in semaphore tests, as well as create image in the api test
- 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should be retained for the same reason I mentioned above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment as above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |