[windows] only test the Swift subset of LLDB tests in CI #85075
Conversation
|
@swift-ci please test windows |
utils/build.ps1
Outdated
| LLVM_UNITTEST_LINK_FLAGS = "$(Get-SwiftSDK -OS Windows -Identifier Windows)\usr\lib\swift\windows\$($Platform.Architecture.LLVMName)\swiftCore.lib"; | ||
| } | ||
|
|
||
| if ($env:ONLY_SWIFT_LLDB_TESTS -eq '1') { |
There was a problem hiding this comment.
I really would rather not introduce this type of behaviour in build.ps1. Can we sink this check into lit itself?
There was a problem hiding this comment.
Would using the check-lldb-swift target be better? https://github.com/swiftlang/llvm-project/blob/b6f1a41420ebffc7f043b16080ef0e8afde4d2cc/lldb/test/CMakeLists.txt#L302-L306
we could have lldb and lldb-swift in the list of tests that build.ps1 supports
|
I opened an alternative PR here: swiftlang/llvm-project#11695 |
charles-zablit
force-pushed
the
charles-zablit/windows/lldb-only-run-swift-test-subset
branch
from
2be681e to
5e5b70e
Compare
utils/build.ps1
Outdated
| DownloadAndVerify $PinnedBuild "$BinaryCache\$PinnedToolchain.exe" $PinnedSHA256 | ||
|
|
||
| if ($Test -contains "lldb") { | ||
| if ($Test -contains "lldb" || $Test -contains "lldb-swift") { |
There was a problem hiding this comment.
I think that -Or is the preferred spelling here (-Or is the Boolean check, || is the chained operation check.
There was a problem hiding this comment.
Fixed, thanks!
utils/build.ps1
Outdated
| Install-PythonModule "packaging" # For building LLVM 18+ | ||
| Install-PythonModule "setuptools" # Required for SWIG support | ||
| if ($Test -contains "lldb") { | ||
| if ($Test -contains "lldb" | $Test -contains "lldb-swift") { |
There was a problem hiding this comment.
This is wrong - -Or should be preferred, || is acceptable. This is going a bit operation :(
There was a problem hiding this comment.
Sorry about that, I think I did an incorrect copy paste when rebasing. I switched to using -or and -and.
utils/build.ps1
Outdated
| if ($TestLLDB) { | ||
| $Targets += @("check-lldb") | ||
| if ($TestLLDB || $TestLLDBSwift) { | ||
| if (($TestLLDB && $TestLLDBSwift) || $TestLLDB) { |
There was a problem hiding this comment.
I'd like this to be simpler:
-TestLLDB: check-lldb
-TestLLDBSwift: check-lldb-swift
There was a problem hiding this comment.
I tried to match that pattern, please let me know what you think.
There was a problem hiding this comment.
I think that there shouldn't be any other conditions - if you specify -TestSwiftLLDB it should run check-lldb-swift. If you specify -TestLLDB, it should run check-lldb. Either we make this fragile or push the check into the build system. We shouldn't be doing any filtering in this script.
There was a problem hiding this comment.
If we do this, it means build.ps1 runs the Swift subset of test twice by default. check-lldb runs the entirety of the lldb test suite (including the swift tests) whereas check-lldb-swift is equivalent to check-lldb --filter='swift'.
We shouldn't be doing any filtering in this script.
We can't refactor the behavior of check-lldb to exclude swift tests as per Adrian's comment.
We can't add the env variable check in lit either because the config is evaluated after the tests have been filtered out.
If we can't add this small extra bit of logic in build.ps1, I'm not sure where we could add it.
I don't think running the swift subset of tests twice by default is a good change either.
charles-zablit
force-pushed
the
charles-zablit/windows/lldb-only-run-swift-test-subset
branch
from
5e5b70e to
1a1a180
Compare
charles-zablit
force-pushed
the
charles-zablit/windows/lldb-only-run-swift-test-subset
branch
from
1a1a180 to
3df36f4
Compare
|
With the latest changes, by default, following the discussion in this thread, we run the Swift subset of lldb tests twice: once in |
2 participants
We cannot break upstream
lldbtests fromswiftlang/swift. This patch adds thecheck-lldb-swifttarget through thelldb-swifttest, which will only check the swift subset of lldb tests.rdar://162128596