Skip to content
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

Add CTS macro for marking tests as known failures in code #2392

Closed
wants to merge 1 commit into from

Conversation

kbenzie
Copy link
Contributor

@kbenzie kbenzie commented Nov 27, 2024

The new UUR_KNOWN_FAILURE_ON macro can be used to skip tests on devices where the test is known to fail. This can be done for a devices in an adapter or by substring match of the device name. For example:

UUR_KNOWN_FAILURE_ON(uur::OpenCL{"Intel(R) UHD Graphics 770"});

This invocation is also used in a few places in order to have clean CTS runs on machines with this iGPU.

@github-actions github-actions bot added the conformance Conformance test suite issues. label Nov 27, 2024
@kbenzie kbenzie force-pushed the benie/cts-unsupported-macro branch from 83dad48 to 0ea7e17 Compare November 29, 2024 11:07
@kbenzie kbenzie changed the title Add CTS macro for marking tests unsupported in code Add CTS macro for marking tests as known failures in code Nov 29, 2024
@kbenzie kbenzie marked this pull request as ready for review November 29, 2024 11:09
@kbenzie kbenzie requested a review from a team as a code owner November 29, 2024 11:09
The new `UUR_KNOWN_FAILURE_ON` macro can be used to skip tests on
devices where the test is known to fail. This can be done for a devices
in an adapter or by substring match of the device name. For example:

```cpp
UUR_KNOWN_FAILURE_ON(uur::OpenCL{"Intel(R) UHD Graphics 770"});
```

This invocation is also used in a few places in order to have clean CTS
runs on machines with this iGPU.
@kbenzie kbenzie force-pushed the benie/cts-unsupported-macro branch from ba57ce9 to 5e376ec Compare December 3, 2024 16:53
@github-actions github-actions bot added the specification Changes or additions to the specification label Dec 3, 2024
deviceName); \
const char *alsoRunKnownFailures = \
std::getenv("UR_CTS_ALSO_RUN_KNOWN_FAILURES"); \
if (alsoRunKnownFailures && (alsoRunKnownFailures == "1"sv || \
Copy link
Contributor

Choose a reason for hiding this comment

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

Could getting and checking the environment variable be done in a separate function? Just to avoid having a huge macro in each failing test.

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've done this in another branch which is based on top of the work to support multiple adapters/paltforms/devices at once. This work will likely get rolled into those changes.

if (result != UR_RESULT_SUCCESS) {
return result;
}
out_value = std::string(data.data(), data.size());
out_value = value.substr(0, value.find_last_of('\0'));
Copy link
Contributor

Choose a reason for hiding this comment

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

A malfunctioning adapter could return a string that isn't null terminated, resulting in a crash here.

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'll chuck a std::min(value.find_last_of('\0'), value.size()) on this.

@aarongreig
Copy link
Contributor

does this have a way to account for parameter specific fails? we still have a lot of expected fails in GetInfo tests

@kbenzie
Copy link
Contributor Author

kbenzie commented Dec 6, 2024

does this have a way to account for parameter specific fails? we still have a lot of expected fails in GetInfo tests

You mean based on GetParam() in parameterised tests?

@aarongreig
Copy link
Contributor

does this have a way to account for parameter specific fails? we still have a lot of expected fails in GetInfo tests

You mean based on GetParam() in parameterised tests?

yeah kind of, unfortunately we have this hack that's going to be hard to get rid of:

const T &getParam() const { return std::get<1>(this->GetParam()); }
in most (maybe all) parameterized tests GetParam() gets you a std::tuple containing the device and whatever T the parameter is, getParam just gets you the parameter

@kbenzie
Copy link
Contributor Author

kbenzie commented Dec 6, 2024

does this have a way to account for parameter specific fails? we still have a lot of expected fails in GetInfo tests

You mean based on GetParam() in parameterised tests?

yeah kind of, unfortunately we have this hack that's going to be hard to get rid of:

const T &getParam() const { return std::get<1>(this->GetParam()); }

in most (maybe all) parameterized tests GetParam() gets you a std::tuple containing the device and whatever T the parameter is, getParam just gets you the parameter

Okay sure, or getParam(). But yes, since its just code we can do stuff like:

if (getParam() == UR_DEVICE_INFO_GLOBAL_MEM_FREE) {
  UUR_KNOWN_FAILURE_ON(uur::LevelZeroV2{});
}

@isaacault
Copy link
Contributor

does this have a way to account for parameter specific fails? we still have a lot of expected fails in GetInfo tests

You mean based on GetParam() in parameterised tests?

yeah kind of, unfortunately we have this hack that's going to be hard to get rid of:

const T &getParam() const { return std::get<1>(this->GetParam()); }

in most (maybe all) parameterized tests GetParam() gets you a std::tuple containing the device and whatever T the parameter is, getParam just gets you the parameter

Okay sure, or getParam(). But yes, since its just code we can do stuff like:

if (getParam() == UR_DEVICE_INFO_GLOBAL_MEM_FREE) {
  UUR_KNOWN_FAILURE_ON(uur::LevelZeroV2{});
}

As well, the need for this shouldn't be quite as prevalent after #2290 is addressed.

@kbenzie
Copy link
Contributor Author

kbenzie commented Dec 12, 2024

I've closing this as the changes will be incorporated into the work @aarongreig is doing to support multi-platform/device testing which will also remove the match files.

@kbenzie kbenzie closed this Dec 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
conformance Conformance test suite issues. specification Changes or additions to the specification
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants