-
Notifications
You must be signed in to change notification settings - Fork 15.2k
[NFC][LLVM-Tests] Specialize test suite for LLVM unit tests #161442
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/27/builds/16894 Here is the relevant piece of the build log for the reference
|
I wonder this would suppress |
It should not, so not intentional. I think these tests should still be a part of check-all atleast and check-llvm as well. Let me test a little bit to confirm tomorrow |
IIRC check-llvm and check-all are their own standalone targets independent of the check-llvm-* targets, but will check |
Here's the command generated for check-llvm:
So, this will run everything under llvm/test, including the unit tests. Locally I do see several |
But for additional verification I created this PR with a broken unit test, expecting that it will fail the checks: #161623 |
Could you try again with cleaning unittests?
|
It does not. And isn't that expected? It's no longer a part of |
I expect e.g. https://lab.llvm.org/buildbot/#/builders/199/builds/6235/steps/12/logs/stdio
|
I see, so I think we need to add UnitTest as a explicit dependency for both |
Ok. in the latest test PR version, I do see that the 2 faulty unit tests do fail:
|
@@ -0,0 +1,5 @@ | |||
add_lit_testsuite(check-llvm-unit "Running lit suite for LLVM unit tests" | |||
${CMAKE_CURRENT_BINARY_DIR} | |||
EXCLUDE_FROM_CHECK_ALL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is it unconditional EXCLUDE_FROM_CHECK_ALL
? It looks conditional on the left.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See similar PR for the earlier specialized suites: #159781
Without EXCLUDE_FROM_CHECK_ALL
, these tests run twice, which is wasteful and sometimes leading to failure due to concurrent execution (which will likely not affect these unit tests).
… (#161623) Add `UnitTests` as an explicit dependency for `check-llvm` and `llvm-test-depends`. In #161442, the intent was to remove `UnitTests` as a dependency for the individual per-directory `check-llvm-*` test suites created but not to drop it from `check-llvm` or `llvm-test-depends`. This missing dependency will cause LLVM unit tests to be not rebuilt and resulting in either `warning: test suite 'LLVM-Unit' contained no tests` or running stale running versions of the unit tests when running `check-llvm`.
…ed by 0e14973 (#161623) Add `UnitTests` as an explicit dependency for `check-llvm` and `llvm-test-depends`. In llvm/llvm-project#161442, the intent was to remove `UnitTests` as a dependency for the individual per-directory `check-llvm-*` test suites created but not to drop it from `check-llvm` or `llvm-test-depends`. This missing dependency will cause LLVM unit tests to be not rebuilt and resulting in either `warning: test suite 'LLVM-Unit' contained no tests` or running stale running versions of the unit tests when running `check-llvm`.
Remove
UnitTests
from LLVM_TEST_DEPENDS_COMMON and create a specialized lit suite for unit-tests and that depends only onUnitTests
.