Skip to content

Conversation

ayylol
Copy link
Contributor

@ayylol ayylol commented Oct 31, 2024

Added %{run} to lit lines that execute the test binary.
Same change as #15636 to some tests that were added recently.

@ayylol
Copy link
Contributor Author

ayylol commented Oct 31, 2024

@sarnex FYI, you mentioned adding a test to enforce this change. I think it would be good, but from what I can tell these should be caught by #15728 when using build only mode, because it will try to execute the tests without the proper devices.

@ayylol ayylol marked this pull request as ready for review October 31, 2024 15:54
@ayylol ayylol requested a review from a team as a code owner October 31, 2024 15:54
@sarnex
Copy link
Contributor

sarnex commented Oct 31, 2024

@ayylol Will it fail in a way where a user will know how to fix it? If not, is it possible to add that?

@ayylol
Copy link
Contributor Author

ayylol commented Oct 31, 2024

@ayylol Will it fail in a way where a user will know how to fix it? If not, is it possible to add that?

It certainly wouldn't be as spelled out as having a test like no_sycl_hpp_in_e2e_tests.cpp failing. But looking at the logs you would be able to tell since it would fail on the build stage on a lit line that executes the test binary, which we don't expect to happen at that stage.

I would still be in favor for adding a test like this, but currently I have not had any good ideas as to how this could be done. One thing that prevents us from just using grep like the other e2e test requirements checks is the fact that tests are not constrained to name their binaries as one specific thing (could be %t, %t1.out, %t2.exe to name some examples), and the fact that we can use the binary in different ways without executing, (for example we could do something like mv %t ...). Would be open to ideas.

@againull againull merged commit 73adce2 into intel:sycl Oct 31, 2024
14 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