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

[cts] Generate conformance tests for adapters #799

Merged
merged 1 commit into from
Aug 18, 2023

Conversation

wlemkows
Copy link
Contributor

No description provided.

@wlemkows wlemkows changed the title Generate conformance tests for adapters [cts] Generate conformance tests for adapters Aug 11, 2023
test/conformance/CMakeLists.txt Outdated Show resolved Hide resolved
WORKING_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR})
else()
if(UR_BUILD_ADAPTER_CUDA)
add_test_force_adapters(${name} adapter_cuda)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd just pass "cuda" and "hip" and "level_zero" as a second param.

The word "adapter_" seems redundant... unless you made it on purpose (e.g. to filter out adapters' related tests), then you can move this "adapter_" word into the function 😉

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This prefix is used intentionally, in order to have the correct name for linking adapters and the correct label. In the function we have several uses for this variable, so let me leave it as it is for now. Perhaps in the future, I will change this part of the code.

Copy link
Contributor

@lukaszstolarczuk lukaszstolarczuk Aug 16, 2023

Choose a reason for hiding this comment

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

Heh, ok, it's not that big of a deal.
But on the other hand, it's just a matter of prepending adapter variable with "adapter_" within the function.

test/conformance/CMakeLists.txt Outdated Show resolved Hide resolved
test/conformance/adapters/hip/CMakeLists.txt Show resolved Hide resolved
test/conformance/CMakeLists.txt Outdated Show resolved Hide resolved
test/conformance/adapters/cuda/CMakeLists.txt Show resolved Hide resolved
Copy link
Contributor

@lukaszstolarczuk lukaszstolarczuk left a comment

Choose a reason for hiding this comment

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

👍

@veselypeta
Copy link
Contributor

veselypeta commented Aug 16, 2023

I'm not sure I fully understand what you're trying to do here? Is this so that the conformance tests will be run with ctest against each adapter present?

@pbalcer
Copy link
Contributor

pbalcer commented Aug 17, 2023

I'm not sure I fully understand what you're trying to do here? Is this so that the conformance tests will be run with ctest against each adapter present?

That's right. We needed the ability to select which adapter is being tested at runtime (not configure-time or build-time). And, maybe more importantly, filter out the failing tests. This approach will allow us to easily run cts for each adapter. It also prepares ctests for upcoming changes where we filter out gtest output to allow known-failing tests that are specific for each adapter. Something like:
COMMAND match.py ${TEST_TARGET_NAME} expected-output-${adapter}.match. This would run the tests and expect a specific output for an adapter, passing even with known failures.

Ideally I'd have preferred for all adapter-specific code to be self-contained (so that we don't have to remember to add if (CUDA) {...} in ctests), but we can improve that later.

Copy link
Contributor

@veselypeta veselypeta left a comment

Choose a reason for hiding this comment

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

Thanks for the clarification @pbalcer.

@pbalcer pbalcer merged commit f7a1a7a into oneapi-src:adapters Aug 18, 2023
36 checks passed
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.

4 participants