Skip to content

Add test-lavapipe and test-warp#1372

Open
joaosaffran wants to merge 4 commits into
llvm:mainfrom
joaosaffran:lavapipe/add-run-only-flags
Open

Add test-lavapipe and test-warp#1372
joaosaffran wants to merge 4 commits into
llvm:mainfrom
joaosaffran:lavapipe/add-run-only-flags

Conversation

@joaosaffran

Copy link
Copy Markdown
Contributor

Add test-lavapipe and test-warp to only run lavapipe and warp tests

@joaosaffran joaosaffran added the test-lavapipe Run lavapipe tests, only label Jul 14, 2026
@joaosaffran joaosaffran marked this pull request as ready for review July 14, 2026 20:44
@joaosaffran joaosaffran added the test-warp Run warp tests, only label Jul 14, 2026
@joaosaffran joaosaffran marked this pull request as draft July 14, 2026 21:03
@joaosaffran joaosaffran marked this pull request as ready for review July 14, 2026 21:07
contains(github.event.pull_request.labels.*.name, 'test-all')
|| contains(github.event.pull_request.labels.*.name, 'test-warp')
|| (!contains(github.event.pull_request.labels.*.name, 'test-lavapipe')
&& !contains(github.event.pull_request.labels.*.name, 'test-warp'))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
&& !contains(github.event.pull_request.labels.*.name, 'test-warp'))
&& !contains(github.event.pull_request.labels.*.name, 'test-warp')

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pretty sure that parentheses goes with the ( that's before !contains in the previous line

|| (!contains(github.event.pull_request.labels.*.name, 'test-lavapipe')
      && !contains(github.event.pull_request.labels.*.name, 'test-warp'))

Comment on lines +20 to +23
if: >-
contains(github.event.pull_request.labels.*.name, 'test-all')
|| (!contains(github.event.pull_request.labels.*.name, 'test-lavapipe')
&& !contains(github.event.pull_request.labels.*.name, 'test-warp'))

@Icohedron Icohedron Jul 14, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I find it odd that test-warp and test-lavapipe behave very differently from test-dx-all and test-vk-all.
test-dx-all and test-vk-all do not disable the default set of tests, while here you are configuring test-warp and test-lavapipe to disable them.

Furthermore, the way test-warp and test-lavapipe are handled will lead to even more difficult-to-read if-condition soup if we decide to add even more labels like it (e.g., test-amd, test-nv, etc.).

I would prefer to have a label such as disable-default-tests to disable the default tests. Then have test-warp and test-lavapipe to selectively enable warp and lavapipe tests.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I guess we could also just change test-dx-all and test-vk-all to also disable the default set of tests.
Just as long as it's consistent and we can reduce the amount of complexity when it comes to managing all the if conditions so there isn't conflicting/differing logic between the test- labels.

@kmpeng kmpeng left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Did we decide these labels are just temporary? If they are we should probably write that in the title and/or description

contents: read
checks: write
if: >-
contains(github.event.pull_request.labels.*.name, 'test-all')

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is the condition contains(github.event.pull_request.labels.*.name, 'test-all') needed for the tier 1 targets + macos?

@Icohedron Icohedron Jul 14, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

No, the condition here is that the tests run if either

  1. The test-all label is there, or
  2. The labels test-warp and test-lavapipe are both not present.

This condition will get even bigger if we decide to make other test-<gpu> labels that disable default tests.
Thus, I proposed having a disable-default-tests label instead of this more difficult-to-read if condition.

Comment on lines +95 to +97
if: >-
contains(github.event.pull_request.labels.*.name, 'test-all')
|| contains(github.event.pull_request.labels.*.name, 'test-lavapipe')

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

test-all already runs Lavapipe - we don't want Exec-Tests-Extra-Lavapipe to run a duplicate

Suggested change
if: >-
contains(github.event.pull_request.labels.*.name, 'test-all')
|| contains(github.event.pull_request.labels.*.name, 'test-lavapipe')
if: >-
contains(github.event.pull_request.labels.*.name, 'test-lavapipe')
&& !contains(github.event.pull_request.labels.*.name, 'test-all')

contains(github.event.pull_request.labels.*.name, 'test-all')
|| contains(github.event.pull_request.labels.*.name, 'test-warp')
|| (!contains(github.event.pull_request.labels.*.name, 'test-lavapipe')
&& !contains(github.event.pull_request.labels.*.name, 'test-warp'))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think you can drop && !contains(github.event.pull_request.labels.*.name, 'test-warp')
If this is true, then the previous condition would be true, and the or would resolve to true.

Comment thread .github/workflows/pr-matrix.yaml
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

test-lavapipe Run lavapipe tests, only test-warp Run warp tests, only

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants