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.

Sign up for GitHub

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

Jump to bottom

[SYCL][E2E] Remove RUNx lines #16032

Open
wants to merge 5 commits into
base: sycl
Choose a base branch
from
Open

[SYCL][E2E] Remove RUNx lines #16032

wants to merge 5 commits into from

Conversation

KornevNikita
Copy link
Contributor

@KornevNikita KornevNikita commented Nov 8, 2024 •
edited
Loading

This PR updates the tests containing RUNx lines.
Some of them are turned back on.
The rest is updated.

@KornevNikita
[SYCL][E2E] Add test to track RUNx tests
e5436bf 
The same idea as for XFAIL and UNSUPPORTED. The test checks if there is
a RUNx without tracker. Also:
- this PR also updates some RUNx tests as they are likely fixed.
- marks some as UNSUPPORTED as there is only one RUNx line.
- adds tracker for the rest.

After that there shouldn't be any untracked RUNx tests in sycl/test-e2e.
@KornevNikita KornevNikita requested review from a team as code owners November 8, 2024 15:12
@KornevNikita KornevNikita requested review from bso-intel and AlexeySachkov and removed request for bso-intel November 8, 2024 15:12
@KornevNikita
Copy link
Contributor Author

Didn't remove @bso-intel, that was made automatically.

@ayylol
Copy link
Contributor

ayylol commented Nov 8, 2024

Hey, just a comment. Unlike the other lit directives that you've been adding the requirements for trackers, RUNx is not actually recognized by our scripts as anything, it just works because it is a misspelling of RUN so it isn't picked up (We could use RUNz just the same).

Should this somehow be documented or enforced (unsure how this could be done) as the way to disable specific run lines?

For example we had this related issue #14902, where a line that was meant to be disabled ran because the misspelling of RUN was still picked up as a RUN: directive.

@bader
Copy link
Contributor

bader commented Nov 8, 2024

Hey, just a comment. Unlike the other lit directives that you've been adding the requirements for trackers, RUNx is not actually recognized by our scripts as anything, it just works because it is a misspelling of RUN so it isn't picked up (We could use RUNz just the same).

Should this somehow be documented or enforced (unsure how this could be done) as the way to disable specific run lines?

For example we had this related issue #14902, where a line that was meant to be disabled ran because the misspelling of RUN was still picked up as a RUN: directive.

I would just remove all RUNx: lines from the tests. If code owners think that test scope should be extended, we should file trackers. Having these lines in the test code is useless and impacts readability/maintainability.

@KornevNikita
Copy link
Contributor Author

Not sure if we need a test to check the presence of RUNx lines at all. @bader what do you think?

@bader
Copy link
Contributor

bader commented Nov 11, 2024

Not sure if we need a test to check the presence of RUNx lines at all. @bader what do you think?

I don't see much value in such test. There are many other ways to disable llvm-lit commands by changing keywords. I don't see how can find them with the simple pattern matching.

In my opinion, codeowners should be responsible for the quality of the tests.

@KornevNikita KornevNikita changed the title [SYCL][E2E] Add test to track RUNx tests [SYCL][E2E] Remove RUNx lines Nov 11, 2024
@KornevNikita
Copy link
Contributor Author

@bso-intel could you take a look please?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bader bader bader left review comments

@EwanC EwanC EwanC approved these changes

@AlexeySachkov AlexeySachkov Awaiting requested review from AlexeySachkov

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@KornevNikita @ayylol @bader @EwanC