Increase test coverage - #247
Conversation
When dotest's harness hard-crashes (the intermittent segfault we keep seeing on Linux/Android functionalities), whatever it wrote right before dying can contain a stray NUL byte, and grep treats the whole stream as binary from that point on, printing just "binary file matches" instead of the crash text -- exactly the diagnostic output we need to root-cause it. -a forces grep to keep treating it as text.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4b1bf46810
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| @@ -1,131 +1 @@ | |||
| skip | |||
There was a problem hiding this comment.
When the Linux or Android test workflows run, emptying all five ds2 exclusion lists makes lldb-dotest execute hundreds of known-broken or flaky tests, even though this commit contains no corresponding debugger fixes. The prior lists include TestTypedefArray, explicitly excluded because it fails across platforms, and Android tests excluded because they occasionally hang; a failure propagates through pipefail in run-lldb-tests, while the Android jobs explicitly exit with dotest's status. Restore the known failures and remove only tests verified to pass after the trial run, otherwise routine CI will fail or time out.
Useful? React with 👍 / 👎.
Verified against PR #247's trial run (exclusion lists emptied, every test actually executed): each dropped entry has an explicit PASS/XFAIL line for its full Module.Class.method identifier, not just an absence from the failure list, since several class names share generically- named methods (test_dwarf, test_dwo, etc.) that would otherwise cause false matches across unrelated classes. Entries with no explicit pass or fail evidence (category-skipped, or belonging to categories still in flight when this was written) are left excluded rather than guessed at. linux-aarch64.excluded and android-aarch64.excluded are untouched: aarch64 categories hadn't finished, and the android-aarch64 sanity run's 4 exclusions all still fail exactly as before.
4b1bf46 to
106e18b
Compare
Its exclusion entry was added for aarch64 in 52d1278 under the old "_llgs"-suffixed method name; upstream since dropped that suffix, so the entry silently stopped matching and the test's known flakiness (staggered per-thread SIGSEGV timing racing a shared mutex) was free to surface again, this time on android-x86_64. Fix the stale name and add the current one to android-x86_64.excluded.
…e run test_breakpoint_set_restart_dwarf/dwo (linux-x86_64) regressed: it was dropped from the ds2 list on single-run PASS evidence, but the same test was independently documented as flaky for other platforms in 593b920 and 52d1278 (a race between LLDB's transparent stop-and- restart when setting a breakpoint on a running process and a genuine visible stop). Re-exclude it, and add test_detach_resumes, which times out waiting for an exit-file signal after detach on both linux-x86_64 and linux-i686. TestFrameRecognizer.* and TestRestartBug.test aren't ds2 issues at all: the former hits an LLDB FrameRecognizer scripting API mismatch, and the latter crashes entirely inside LLDB's own mock gdb-remote test harness (gdbclientutils.py) without ever reaching ds2. Both go in the upstream lists instead of the ds2 ones.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3974786460
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| TestLldbGdbServer.LldbGdbServerTestCase.test_Hg_switches_to_3_threads_launch_llgs | ||
| TestLldbGdbServer.LldbGdbServerTestCase.test_qMemoryRegionInfo_reports_heap_address_as_rw_llgs | ||
| TestLldbGdbServer.LldbGdbServerTestCase.test_qMemoryRegionInfo_reports_stack_address_as_rw_llgs | ||
| TestLldbGdbServer.LldbGdbServerTestCase.test_qSupported_fork_events_llgs |
There was a problem hiding this comment.
In the linux-x86_64 matrix in .github/workflows/test-linux.yml, this deletion re-enables test_qMemoryRegionInfo_reports_stack_address_as_rw_llgs, even though repository commit 4e02352 records that it was excluded after being flaky in nightly runs for several weeks, and the subsequent Sources history contains no relevant memory-region fix. Fresh evidence beyond the earlier blanket exclusion comment is that the final tree selectively restores other flakes exposed by the trial run while leaving this specifically documented flake enabled based on a single PASS. When it recurs, run-lldb-tests propagates the failure through pipefail, so retain this exclusion until the flake is fixed or repeated runs establish stability.
Useful? React with 👍 / 👎.
Verified against PR #247's trial run (exclusion lists emptied, every test actually executed): each dropped entry has an explicit PASS/XFAIL line for its full Module.Class.method identifier, not just an absence from the failure list, since several class names share generically- named methods (test_dwarf, test_dwo, etc.) that would otherwise cause false matches across unrelated classes. Entries with no explicit pass or fail evidence (category-skipped, or belonging to categories still in flight when this was written) are left excluded rather than guessed at. linux-aarch64.excluded and android-aarch64.excluded are untouched: aarch64 categories hadn't finished, and the android-aarch64 sanity run's 4 exclusions all still fail exactly as before.
1 participant
This pull request primarily updates the way test log filtering is handled in CI scripts and removes a large number of previously excluded LLDB test cases for Android and Linux platforms. The key changes include ensuring that
grepalways treats input as text (using-a), and clearing out the exclusion lists for several test configurations, which will allow more tests to run in CI.Test log filtering improvements:
.github/actions/run-lldb-tests/action.ymland.github/workflows/test-android.yml) to usegrep -a -vinstead ofgrep -vwhen filtering out lines starting withUNSUPPORTED: LLDB, ensuring that log files are always treated as text and avoiding issues with binary files. [1] [2] [3]Test exclusion list cleanup:
Support/Testing/Excluded/ds2/android-x86_64.excluded,Support/Testing/Excluded/ds2/android-aarch64.excluded, andSupport/Testing/Excluded/ds2/linux-aarch64.excluded, effectively re-enabling these tests in CI runs for the corresponding platforms.These changes should result in more comprehensive test coverage on CI for Android and Linux AArch64 targets and more robust log filtering.