Skip to content

Conversation

@dmpots
Copy link
Contributor

@dmpots dmpots commented Jul 15, 2025

The plugins completion test was checking completions for the abi plugins. But the available abi plugins will depend on which targets are enabled in the cmake build configuration.

This PR updates the test to check for the json object file instead which should be enabled on all builds.

The plugins completion test was checking completions for the abi
plugins. But the available abi plugins will depend on which [targets](https://github.com/llvm/llvm-project/blob/42d2ae1034b287eb60563c370dbf52c59b66db20/lldb/source/Plugins/ABI/CMakeLists.txt#L7)
are enabled in the cmake build configuration.

This PR updates the test to check for the json object file instead which
should be enabled on all builds.
@dmpots dmpots requested a review from JDevlieghere as a code owner July 15, 2025 20:49
@llvmbot llvmbot added the lldb label Jul 15, 2025
@dmpots
Copy link
Contributor Author

dmpots commented Jul 15, 2025

This is to fix a buildbot failure: https://lab.llvm.org/buildbot/#/builders/195/builds/11884

@llvmbot
Copy link
Member

llvmbot commented Jul 15, 2025

@llvm/pr-subscribers-lldb

Author: David Peixotto (dmpots)

Changes

The plugins completion test was checking completions for the abi plugins. But the available abi plugins will depend on which targets are enabled in the cmake build configuration.

This PR updates the test to check for the json object file instead which should be enabled on all builds.


Full diff: https://github.com/llvm/llvm-project/pull/148956.diff

1 Files Affected:

  • (modified) lldb/test/API/commands/plugin/TestPlugin.py (+4-4)
diff --git a/lldb/test/API/commands/plugin/TestPlugin.py b/lldb/test/API/commands/plugin/TestPlugin.py
index e7de7a3f797f1..bd8ab9604538e 100644
--- a/lldb/test/API/commands/plugin/TestPlugin.py
+++ b/lldb/test/API/commands/plugin/TestPlugin.py
@@ -82,10 +82,10 @@ def test_completions(self):
         )
 
         # A completion for a full namespace should contain the plugins in that namespace.
-        self.completions_contain("plugin list abi", ["abi.sysv-x86_64"])
-        self.completions_contain("plugin list abi.", ["abi.sysv-x86_64"])
-        self.completions_contain("plugin list abi.s", ["abi.sysv-x86_64"])
-        self.completions_contain("plugin list abi.sysv-x", ["abi.sysv-x86_64"])
+        self.completions_contain("plugin list object-file", ["object-file.JSON"])
+        self.completions_contain("plugin list object-file.", ["object-file.JSON"])
+        self.completions_contain("plugin list object-file.J", ["object-file.JSON"])
+        self.completions_contain("plugin list object-file.JS", ["object-file.JSON"])
 
         # Check for a completion that is a both a complete namespace and a prefix of
         # another namespace. It should return the completions for the plugins in the completed

Copy link
Contributor

@Jlalond Jlalond left a comment

Choose a reason for hiding this comment

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

LGTM if object file json will always be present, otherwise gating this test makes sense IMO.

self.completions_contain("plugin list abi.", ["abi.sysv-x86_64"])
self.completions_contain("plugin list abi.s", ["abi.sysv-x86_64"])
self.completions_contain("plugin list abi.sysv-x", ["abi.sysv-x86_64"])
self.completions_contain("plugin list object-file", ["object-file.JSON"])
Copy link
Contributor

Choose a reason for hiding this comment

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

The object file Json plugin will always be there?

Can we gate the test in such a way to ensure that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I do not see anything in the CMakeLists.txt file that conditionally enables the JSON object file plugin (unlike the ABI plugins). I also do not see any gates in the json object file tests:

So I think it should always be there.

@dmpots dmpots merged commit c0785ea into llvm:main Jul 15, 2025
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants